Change 448415 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Separate the LazyLoadedImages transform from MobileFormatter

https://gerrit.wikimedia.org/r/448415

Change 448415 merged by Pmiazga:
[mediawiki/extensions/MobileFrontend@master] Separate the LazyLoadedImages transform from MobileFormatter

https://gerrit.wikimedia.org/r/448415

@ovasileva Possible an important piece of maintenance that @Peter.ovchyn might be able to help with.

@ovasileva I'd be pleased to help :)

In T186823#6357317, @Jdlrobson wrote:

@ovasileva Possible an important piece of maintenance that @Peter.ovchyn might be able to help with.

Sounds good to me

Change 620716 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] Move move doRemoveImages to class RemoveImageTransform

https://gerrit.wikimedia.org/r/620716

Change 621981 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] WIP: Move makeSections to MakeSectionsTransform class

https://gerrit.wikimedia.org/r/621981

Change 620716 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move doRemoveImages to class RemoveImagesTransform

https://gerrit.wikimedia.org/r/620716

Even though this task is done (formally), there's a some of stuff to do next. Dropping this here to form into tasks in the future:

1. Extract makeHeadingsEditable to MakeHeadingsEditableTransfrom class.

2. Move creating of RemoveImagesTransform and MakeSectionsTransform to MobileFormatter::__constructor

At the moment all transforms are created right "in-place" in MobileFormatter::filterContent
This demands for us to keep some variables like MobileFormatter::$title, MobileFormatter::$topHeadingTags etc, only to create Transformers.
If we move creating them to MobileFormatter::__constructor we could avoid that.

At the moment we are blocked by MobileFormatter::$scriptsEnabled that is changed in MobileFormatter::disableScripts.

3. Move conditional to MobileFormatter:: __construct and use polymorphism.

At the moment there's a conditional behavior that depends on some settings like:

if ( $this->removeMedia ) {
    $removeImagesTransform = new RemoveImagesTransform();
    /** @phan-suppress-next-line PhanTypeMismatchArgument DOMNode vs. DOMElement */
    $removeImagesTransform->apply( $doc->getElementsByTagName( 'body' )->item( 0 ) );
}

This demands from us keeping them in class. Once 1 and 2 are done we could simplify this by moving conditions to constructor and use NoTransfrom when we don't need transformation like:

public function __construct(...) {
    if ( $this->removeMedia ) {
        $this->removeImagesTransform = new RemoveImagesTransform();
    } else {
        $this->removeImagesTransform = NoTransform::getInstance();
   }
}
// Then we can call without and conditional so we don't need to keep variable:

public function filterContent( ... ) {
...
        /** @phan-suppress-next-line PhanTypeMismatchArgument DOMNode vs. DOMElement */
        $removeImagesTransform->apply( $doc->getElementsByTagName( 'body' )->item( 0 ) );
...
}

4. Replace Transformers with array:

when 1-3 are don, we will have code like that:

class MobileFormatter extends HtmlFormatter {
    private $removeImagesTransform;
    private $makeHeadingsEditable; 
...
and then..
    public function __construct( ... ) {
       if ( $removeMedia ) {
          $this-> removeImagesTransform = NoTransform::getInstance();
       } else {
          $this->removeImagesTransform = new RemoveImagesTransform();
       }
      // The same as above for other transformers. 
     ...
    }
...
    public function filterContent( ... ) {
        $this->removeImagesTransform->apply();
        $this->makeSectionsTransform->apply();
        ....
    }
}

All above could be replaced with array like:

class MobileFormatter extends HtmlFormatter {
    private $transformers = [];
...
and then..
    public function __construct( ... ) {
       if ( !$removeMedia ) {
          $this->transformers []= new RemoveImagesTransform();
       }
      // The same as above for other transformers. 
     ...
    }
...
    public function filterContent( ... ) {
        foreach ( $this->removeImagesTransform as $transformer) {
            $transformer->apply();
        }
        ....
    }
}

5. Flatten the transformers

At the moment MobileSectionTransform contains nested transformers that transform sections: LazyImageTransform and MoveLeadParagraphTransform
It would be good to extract them out of the MobileSectionTransform and use equally to others. It might be a bit tricky and it will demand working with OOP hierarchies (or like that) but still possible.

After steps 1-5 we'll have 100% generic solution that will work equally with all transformers. All that will allow us change any this piece of functionality quickly. Not sure about safety as in certain ways order matters.

Hey @Peter.ovchyn I had a read through T186823#6409592 and mostly agree with the approach, and do agree with the direction. I think MobileFormatter::filterContent got conflated with HtmlFormatter::filterContent and the execution of transforms should be separated from that method as really it's a function for removing content not transforming. I began https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/622866 to explore what I think the direction should look like.

I think 1 "1. Extract makeHeadingsEditable to MakeHeadingsEditableTransfrom class." should be done as part of this work, however. Is that okay with you? Let's call it SubHeadingTransform. I'm sorry I missed it in the original task description.

Change 623415 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] Move makeHeadingsEditable to SubHeadingTransform class

https://gerrit.wikimedia.org/r/623415

Change 621981 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move makeSections to MakeSectionsTransform class

https://gerrit.wikimedia.org/r/621981

Change 623415 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move makeHeadingsEditable to SubHeadingTransform class

https://gerrit.wikimedia.org/r/623415