Closed Bug 1449791 Opened 2 years ago Closed 2 years ago

Remove XUL Overlays

Categories

(Core :: XUL, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Remove all the platform code supporting XUL overlays.
Priority: -- → P5
Blocks: 1426763
Depends on: 1450757
Waiting to ask for review to give the thunderbird folks some more time and I still need to do one or two more passes on XULDocument for some cleanup.
Depends on: 1450753
No longer depends on: 1450757
Adding a reminder to ask bz for review when he's back.
Flags: needinfo?(bdahl)
Attachment #8970745 - Attachment is obsolete: true
Attachment #8970746 - Attachment is obsolete: true
Attachment #8970747 - Attachment is obsolete: true
Attachment #8970748 - Attachment is obsolete: true
Flags: needinfo?(bdahl)
I'm not sure if you have a preferred way of reviewing code removal, but I have it more broken down in commits in https://hg.mozilla.org/try/rev/ed541d53d3474282b0de571ce62dcd5f7b8fd434. A few of the commits had work in progress, so I think it's easier to just review overall. I'm happy to do it differently if you want though.
How is this coming along? We've removed all overlays from TB in bug 1444468 and friends. ETA for this was end of May (bug 1444468 comment #12).
Comment on attachment 8970744 [details]
Bug 1449791 - Remove platform support of overlays.

https://reviewboard.mozilla.org/r/239482/#review261540

r=me.  This is awesome.  ;)

::: dom/xul/XULDocument.cpp:363
(Diff revision 2)
>          // which will add style sheet clones to each document.
>          bool loaded;
>          rv = proto->AwaitLoadDone(this, &loaded);
>          if (NS_FAILED(rv)) return rv;
>  
> -        mMasterPrototype = mCurrentPrototype = proto;
> +        mCurrentPrototype = proto;

mCurrentPrototype should get renamed to mPrototype, because there is only one of it now, right?  Followup is fine for this.

::: dom/xul/XULDocument.cpp:413
(Diff revision 2)
>  
>      NS_IF_ADDREF(*aDocListener);
>      return NS_OK;
>  }
>  
> -// This gets invoked after a prototype for this document or one of
> +// This gets invoked after a prototype for this document is fully built in the

s/a/the/

::: dom/xul/XULDocument.cpp:457
(Diff revision 2)
>  
>      rv = PrepareToWalk();
>      NS_ASSERTION(NS_SUCCEEDED(rv), "unable to prepare for walk");
>      if (NS_FAILED(rv)) return rv;
>  
>      if (aResumeWalk) {

I suspect the whole OnPrototypeLoadDone/ResumeWalk setup can also become simpler, now that we will really only walk one thing.  Again, followup.

::: dom/xul/XULDocument.cpp:1824
(Diff revision 2)
>  
> -    // Now check the chrome registry for any additional overlays.
> -    rv = AddChromeOverlays();
> -    if (NS_FAILED(rv)) return rv;
> -
>      // Do one-time initialization if we're preparing to walk the

No if.  That's the only thing we walk.

::: dom/xul/XULDocument.cpp:1929
(Diff revision 2)
>  
>      return NS_OK;
>  }
>  
>  nsresult
> -XULDocument::InsertXULOverlayPI(const nsXULPrototypePI* aProtoPI,
> +XULDocument::ResumeWalk()

Boy, mozreview's diff made a hash of this.... The raw diff is much saner.

::: dom/xul/XULDocument.cpp:2021
(Diff revision 2)
> -                    if (NS_FAILED(rv)) return rv;
>                  }
>                  else {
> -                    if (mState == eState_Master) {
> -                        // If there are no children, and we're in the
> +                    // If there are no children, and we're in the
> -                        // master document, do post-order document hookup
> +                    // master document, do post-order document hookup

There is no "master document" here... just one document.
Attachment #8970744 - Flags: review?(bzbarsky) → review+
Blocks: 1474042
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f50dd0905ec
Remove platform support of overlays. r=bz
https://hg.mozilla.org/mozilla-central/rev/5f50dd0905ec
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
See Also: → 1493210
You need to log in before you can comment on or make changes to this bug.