Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: bdahl, Assigned: bdahl)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

a year ago
Remove all the platform code supporting XUL overlays.

Updated

a year ago
Priority: -- → P5
(Assignee)

Updated

a year ago
Blocks: 1426763
Depends on: 1450757
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 1450753
No longer depends on: 1450757
(Assignee)

Comment 7

10 months ago
Adding a reminder to ask bz for review when he's back.
Flags: needinfo?(bdahl)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8970745 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8970746 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8970747 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8970748 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Flags: needinfo?(bdahl)
(Assignee)

Comment 9

10 months ago
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.

Comment 10

10 months ago
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 11

10 months ago
mozreview-review
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: 1473244
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Blocks: 1474042

Comment 13

10 months ago
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f50dd0905ec
Remove platform support of overlays. r=bz

Comment 14

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5f50dd0905ec
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

7 months ago
See Also: → 1493210
You need to log in before you can comment on or make changes to this bug.