Closed
Bug 1322414
Opened 8 years ago
Closed 8 years ago
Move content vs. content-primary distinction out of the `type` browser attribute
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: Gijs, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Keywords: addon-compat)
Attachments
(4 files)
If we want to use type=chrome docshells to load chrome stuff (so we can prohibit loading chrome stuff in type=content docshells) then we need to be able to treat the chrome docshells as targetable/primary. In order to do that, either we have to introduce the targetable/primary distinction to type=chrome browsers, or we have to move that distinction to a separate attribute. The latter seems cleaner, so let's do that.
Comment 1•8 years ago
|
||
See also bug 1310795, but it does look like we still use mPrimaryContentShell for a bunch of stuff...
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1) > See also bug 1310795, but it does look like we still use > mPrimaryContentShell for a bunch of stuff... I was looking at actually fixing this bug, and, uh, I'm confused. Because we read the attribute here and here: https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#918 https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#3271 and then pass it to contentshelladded: https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#966 https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsFrameLoader.cpp#3291-3292 which AIUI ends up here: https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/xpfe/appshell/nsXULWindow.cpp#1701-1702,1708,1710-1713 which is odd because it uses "content-primary" and "content-targetable" as an ID? But all the content browsers have "content-targetable" except the active one... so I don't see how this is even working right now? What's the point of this code? AFAICT we don't have in-tree callers for GetContentShellById ( https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#361 ) but it's still weeeeeird that we're doing this. :-\
Flags: needinfo?(bzbarsky)
Comment 3•8 years ago
|
||
I was just looking at the same thing while working on bug 1310796. I think the whole id thing looks pretty bogus to me and we should remove it. We do have an in-tree caller of GetContentShellById, though: nsWebShellWindow::LoadContentAreas. I'm not sure whether it's ever reached in practice with a URL that has the right bits in it to trigger that codepath.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3) > We do have an in-tree caller of GetContentShellById, though: > nsWebShellWindow::LoadContentAreas. I'm not sure whether it's ever reached > in practice with a URL that has the right bits in it to trigger that > codepath. I don't know of any chrome that does that, and the code seems to have been dead for 15 years. So I assume we can just nuke it, but here's a trypush before I go any further, I guess: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2df941111da33628bb1fc01de1b7bd1c39055fd6
Assignee | ||
Comment 5•8 years ago
|
||
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b195e2fb3b276aa6a030be62dac37d4b5a7e74d
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Summary: Move content-targetable and content-primary distinction out of the `type` browser attribute → Move content vs. content-primary distinction out of the `type` browser attribute
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years ago
|
||
Boris, would you mind reviewing the first two parts, as you've just touched the same code in bug 1310796? :-)
Depends on: 1310796
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 11•8 years ago
|
||
Annnd there's quite a bit of orange - it seems that window.content is null in some circumstances on non-e10s. I expect I missed something somewhere...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #11) > Annnd there's quite a bit of orange - it seems that window.content is null > in some circumstances on non-e10s. I expect I missed something somewhere... I forgot to update the mutation observer to care about updates to the 'primary' attribute. Oops. https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f
Assignee | ||
Comment 16•8 years ago
|
||
(the remaining orange is because of my previous patch for bug 1322609 being broken)
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8817638 [details] Bug 1322414 - part 1 - remove GetContentShellById and id passing, https://reviewboard.mozilla.org/r/97856/#review98386 So after this change, doing ContentShellAdded for non-primary things is just a really fancy and obfuscated way of setting their treeowner, eh? At least if we ignore the fact that we can have someone else implementing a treeowner.... ::: xpfe/appshell/nsWebShellWindow.cpp (Diff revision 1) > - AppendUTF8toUTF16(search, searchSpec); > - } > - } > - } > - > - // content URLs are specified in the search part of the URL I'm hoping we know for a fact that this is dead code. I guess this is for a _chrome_ window url with a query string like that?
Attachment #8817638 -
Flags: review+
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8817639 [details] Bug 1322414 - part 2: use a separate 'primary' attribute for primary child browsers, https://reviewboard.mozilla.org/r/97858/#review98388 Presumably you'll merge parts 3 and 4 in here, so we don't have the temporary broken state? ::: dom/base/nsFrameLoader.cpp:917 (Diff revision 2) > { > NS_PRECONDITION(aItem, "Must have docshell treeitem"); > NS_PRECONDITION(mOwnerContent, "Must have owning content"); > > nsAutoString value; > - bool isContent = false; > + bool isContent = mOwnerContent->AttrValueIs( This might break extensions that use "content-something" values, no? Do we know for a fact that this doesn't happen? Yes, I should have restricted this to "content-targetable" and "content-primary" back when. :( I guess I'm OK with this change as long as we loop Jorge in on it.
Attachment #8817639 -
Flags: review+
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17) > Comment on attachment 8817638 [details] > Bug 1322414 - part 1 - remove GetContentShellById and id passing, > > https://reviewboard.mozilla.org/r/97856/#review98386 > > So after this change, doing ContentShellAdded for non-primary things is just > a really fancy and obfuscated way of setting their treeowner, eh? At least > if we ignore the fact that we can have someone else implementing a > treeowner.... Yes, and ContentShellRemoved + ContentShellAdded is mostly obfuscated 'type' and primary switching. It probably makes sense to have 2-3 separate methods to do these 2-3 separate things. I'll file a followup bug if/when this stuff lands successfully... > ::: xpfe/appshell/nsWebShellWindow.cpp > (Diff revision 1) > > - AppendUTF8toUTF16(search, searchSpec); > > - } > > - } > > - } > > - > > - // content URLs are specified in the search part of the URL > > I'm hoping we know for a fact that this is dead code. That's hard to do, given that URLs are basically just random strings in (as well as outside!) our codebase. :-\ Basically, the code here hasn't changed since before 2003, besides refactorings like nsbool vs bool and the like, which seems like a reasonable indication that nobody actually uses it - but perhaps my impression is coloured by the 'recency' of what I usually work on? The other indication that this is dead is that, as far as I understand, for it to work, there'd need to be code that has type=content-<unique-identifier> frames. I looked for such frames in m-c (basically, used dxr to search for lines that contained both 'type' and 'content-', and then filtered out 'content-length', 'content-type' etc.) and didn't find any. > I guess this is for a > _chrome_ window url with a query string like that? As far as I can tell, yes.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bzbarsky)
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8817640 [details] Bug 1322414 - part 3: use primary attribute in frontend, https://reviewboard.mozilla.org/r/97860/#review98652
Attachment #8817640 -
Flags: review?(mconley) → review+
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8817641 [details] Bug 1322414 - part 4: fix test usage of type=content-primary to use type=content and primary=true, https://reviewboard.mozilla.org/r/97862/#review98662 This all looks okay to me - though I didn't do a searchfox/dxr to make sure you hit all of the spots. I assume that automation will catch that.
Attachment #8817641 -
Flags: review?(mconley) → review+
Comment 22•8 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6a73f37649 part 1 - remove GetContentShellById and id passing, r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c689dbcd92 part 2,3,4: use a separate 'primary' attribute for primary child browsers, r=bz,mconley
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a6a73f37649 https://hg.mozilla.org/mozilla-central/rev/e3c689dbcd92
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 24•8 years ago
|
||
Hmm, that completely smashed Thunderbird without notice so we fixed it here: https://hg.mozilla.org/comm-central/rev/42aa2df379ce3ffa520f01c2799a9e4283c45327 We changed type="content-targetable" to type="content" and type="content-primary" to type="content" and primary="true". What about "content-conversation"? http://searchfox.org/comm-central/search?q=content-conversation&case=true®exp=false&path=
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #24) > Hmm, that completely smashed Thunderbird without notice so we fixed it here: > https://hg.mozilla.org/comm-central/rev/ > 42aa2df379ce3ffa520f01c2799a9e4283c45327 > > We changed type="content-targetable" to type="content" and > type="content-primary" to type="content" and primary="true". > > What about "content-conversation"? > http://searchfox.org/comm-central/search?q=content- > conversation&case=true®exp=false&path= It wants to be type="content". If it was never set to type="content-primary" before, I suspect that's all you need to do.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 26•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #15) > (In reply to :Gijs Kruitbosch from comment #11) > > Annnd there's quite a bit of orange - it seems that window.content is null > > in some circumstances on non-e10s. I expect I missed something somewhere... > > I forgot to update the mutation observer to care about updates to the > 'primary' attribute. Oops. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f Hi, what is the plan to fix this? Have you made a followup bug?
Assignee | ||
Comment 27•8 years ago
|
||
(In reply to :aceman from comment #26) > (In reply to :Gijs Kruitbosch from comment #15) > > (In reply to :Gijs Kruitbosch from comment #11) > > > Annnd there's quite a bit of orange - it seems that window.content is null > > > in some circumstances on non-e10s. I expect I missed something somewhere... > > > > I forgot to update the mutation observer to care about updates to the > > 'primary' attribute. Oops. > > > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f > > Hi, what is the plan to fix this? Have you made a followup bug? This was on try. I updated the patches and only the updated patches landed (and were green, or they would have been backed out already).
Comment 28•7 years ago
|
||
This change breaks the RDP Inspector addon - see this issue: https://github.com/firebug/rdp-inspector/issues/88 It's bad that there is no way to set type="content-primary" in a way that works both in 52 and 53. The RDP Inspector seems to work well even with primary="false". But I'm not 100% sure about that and maybe there are other addons that really require primary="true".
Assignee | ||
Comment 29•7 years ago
|
||
(In reply to Jarda Snajdr [:jsnajdr] from comment #28) > This change breaks the RDP Inspector addon - see this issue: > > https://github.com/firebug/rdp-inspector/issues/88 > > It's bad that there is no way to set type="content-primary" in a way that > works both in 52 and 53. The RDP Inspector seems to work well even with > primary="false". But I'm not 100% sure about that and maybe there are other > addons that really require primary="true". It can detect whether these changes are present on a build it's running in using the presence or lack thereof of the getContentShellById API on nsIXULWindow. You only need the primary/non-primary distinction if you've got more than one child content docshell in your window, which from a quick look your add-on doesn't seem to. If you did have more than 1 kid docshell you'd presumably change the 'primary' at runtime anyway, so the extra check in JS is relatively easy to implement. That said, it looks like content-primary is used relatively frequently by add-ons (711 hits on DXR). I don't really understand why, several of the cases where add-ons check for those values rather than using them themselves look like broken ways of checking "is this a main firefox tabbrowser browser", but yeah... I'm not sure how much we should care given XUL add-ons are going away and the distance to release for this change, plus the fact that all the add-ons I've spot-checked so far are unmaintained. I'm going to need time I don't have right now to go through the list properly... Unfortunately the checks for these attributes are more spread out than one might like, and so I would prefer not to support both syntaxes if we can help it, to avoid the code becoming hairier than it needs to be. I could add support for "content-*" meaning "content" again, but it's also unclear how many of the add-ons that would fix. I'll come back to this when my request queue is less depressing. Meanwhile, Boris, thoughts about the above and whether we *need* to re-add some complexity to support both syntaxes? (Clearly, based on comment #18, I should have looked at this before - not sure why that fell through the cracks, but my apologies either way... :-( )
Comment 30•7 years ago
|
||
Did we ever ping Jorge about this? I do think it makes sense to support "content-*" meaning "content" for a bit while people migrate. It would allow <browser type="content-primary" primary="true"> as a transitional thing. I can't speak to the support status for XUL addons and timeframes; I haven't really been tracking that. The addons that are _checking_ for content-primary are a bigger problem; those probably just need to be updated.
Flags: needinfo?(bzbarsky) → needinfo?(jorge)
Assignee | ||
Comment 31•7 years ago
|
||
I've put up a patch in the dep.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 32•7 years ago
|
||
Supporting "content-*" as "content" at least for a couple of cycles would be ideal, I agree.
Flags: needinfo?(jorge)
You need to log in
before you can comment on or make changes to this bug.
Description
•