Status

()

enhancement
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bdahl, Assigned: bdahl)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
The first stage of removing the platform code to support overlays:

1) crash on loading overlays
2) remove all overlay tests
(Assignee)

Updated

a year ago
Assignee: nobody → bdahl

Comment 1

a year ago
We're busily removing overlays from Thunderbird, but it might take us another week before we're done. Perhaps you could hide the crash behind a preference so we get a bit more time to complete the work.
(Assignee)

Comment 2

a year ago
(In reply to Jorg K (GMT+1) from comment #1)
> We're busily removing overlays from Thunderbird, but it might take us
> another week before we're done. Perhaps you could hide the crash behind a
> preference so we get a bit more time to complete the work.

That or I can use MOZ_BUILD_APP. Do you have a meta bug tracking XUL overlay removal in Thunderbird?
Flags: needinfo?(jorgk)

Comment 3

a year ago
Yes, bug 1444468. I need to link a few we have removed there.
Flags: needinfo?(jorgk)
(Assignee)

Updated

a year ago
Blocks: 1449791
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f1ba8b85d492b6ca2cffca9768f3de5da5dd84

I tried to be as thorough as possible, but there were quite a few things to remove.

Comment 7

a year ago
mozreview-review
Comment on attachment 8963429 [details]
Bug 1448162 - Remove all XUL overlay tests.

https://reviewboard.mozilla.org/r/232330/#review237928

::: commit-message-de322:1
(Diff revision 1)
> +BUg 1448162 - Remove all XUL overlay tests. r?gijs

Nitty nit: "Bug"

::: chrome/test/unit/test_bug564667.js
(Diff revision 1)
> -  // Test Adding Override
> -  test_mapping("chrome://testOverride/content", "file:///test1/override");

This looks like a test for a chrome url override, not a XUL overlay? So it should probably be kept.

::: layout/reftests/xul-document-load/readme.txt
(Diff revision 1)
> -test010: PIs in the master document, outside the prolog, don't have any effect but get
> -         added to the DOM.

This test looks to me like it ought not to be deleted, but have the overlay file removed, and the overlay reference removed, and the overlay checks in the test removed -- but the stylesheet PI checks should be kept as they continue to be correct and reasonable (AFAICT).

::: layout/reftests/xul-document-load/readme.txt:24
(Diff revision 1)
>  test014: (bug 363406) Relative URIs in overlay's <?xul-overlay ?> PI are resolved
>           against overlay's URI, not the document URI.

Looks like we should remove the mention of this test (which is correctly being removed), too?
Attachment #8963429 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 8

a year ago
Gah, forgot to point this out in my topline review comment: I'm not technically a dom/xul peer. So I think you need review from someone else...

Updated

a year ago
Attachment #8963430 - Flags: review?(gijskruitbosch+bugs) → review?(nika)

Comment 9

a year ago
Nika is probably a better reviewer for the crash patch, possibly also to check over the test removal.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8963430 [details]
Bug 1448162 - Crash in the presence of a XUL overlay.

https://reviewboard.mozilla.org/r/232332/#review238932

r=me with the changes I requested

::: dom/xul/XULDocument.cpp:2262
(Diff revision 3)
>                                   bool* aShouldReturn,
>                                   bool* aFailureFromContent)
>  {
>      nsresult rv;
>  
> +#ifdef MOZ_BUILD_APP_IS_BROWSER

Please add a comment referencing this bug and explaining why we enable this based on what MOZ_BUILD_APP is.

::: dom/xul/XULDocument.cpp:2268
(Diff revision 3)
> +    nsCString docSpec;
> +    mCurrentPrototype->GetURI()->GetSpec(docSpec);
> +    nsCString overlaySpec;
> +    aURI->GetSpec(overlaySpec);
> +    printf("Attempt to load overlay %s into %s\n",
> +           PromiseFlatCString(overlaySpec).get(),

PromiseFlatCString is completely unnecessary here and below, because nsCString is already flat (and thus exposes the .get() method).

::: dom/xul/moz.build:11
(Diff revision 3)
>  
>  with Files("**"):
>      BUG_COMPONENT = ("Core", "XUL")
>  
> +if CONFIG['MOZ_BUILD_APP'] == 'browser':
> +    DEFINES['MOZ_BUILD_APP_IS_BROWSER'] = True

This seems like an odd define to make locally in dom/xul...

How about we call it `MOZ_CRASH_XUL_OVERLAYS` instead?
Attachment #8963430 - Flags: review?(nika) → review+
Thanks for putting this behind an ifdef! I'm sorry if I missed this, but even without Thunderbird, what is the rationale behind crashing in this case? Wouldn't it make more sense to show errors or warnings, instead of crashing the whole product?
Or you could if on the MOZ_THUNDERBIRD (and MOZ_SUITE) define.
(Assignee)

Comment 17

a year ago
(In reply to Philipp Kewisch [:Fallen]  from comment #15)
> Thanks for putting this behind an ifdef! I'm sorry if I missed this, but
> even without Thunderbird, what is the rationale behind crashing in this
> case? Wouldn't it make more sense to show errors or warnings, instead of
> crashing the whole product?

In Firefox I want to ensure no one adds any new overlays. An error or warning would probably be okay, but they often go unnoticed.
This is ideally something to be caught during a review. This is just my opinion, but I think a crash is a bit harsh. I probably would have gone with an error message and then just not loading the overlay. Anyway, given this code after the crash will likely go away very soon now it is probably not worth discussing the exact approach. Thanks again!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

a year ago
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee4c52cd9d53
Remove all XUL overlay tests. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/61c57ad4f4f4
Crash in the presence of a XUL overlay. r=mystor

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee4c52cd9d53
https://hg.mozilla.org/mozilla-central/rev/61c57ad4f4f4
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1451625
You need to log in before you can comment on or make changes to this bug.