Closed Bug 1448162 Opened 6 years ago Closed 6 years ago

Disable XUL overlays

Categories

(Core :: XUL, enhancement)

Product:
Core
Component:
XUL
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(2 files)

Bug 1448162 - Remove all XUL overlay tests.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1448162 - Crash in the presence of a XUL overlay.
59 bytes, text/x-review-board-request
nika
: review+
Details 
The first stage of removing the platform code to support overlays:

1) crash on loading overlays
2) remove all overlay tests
Assignee: nobody → bdahl
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.
(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)
Yes, bug 1444468. I need to link a few we have removed there.
Flags: needinfo?(jorgk)
Blocks: 1449791
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 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+
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...
Attachment #8963430 - Flags: review?(gijskruitbosch+bugs) → review?(nika)
Nika is probably a better reviewer for the crash patch, possibly also to check over the test removal.
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.
(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!
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
https://hg.mozilla.org/mozilla-central/rev/ee4c52cd9d53
https://hg.mozilla.org/mozilla-central/rev/61c57ad4f4f4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1451625
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: