Closed
Bug 720799
Opened 12 years ago
Closed 12 years ago
Don't use mDocShell in nsScreen
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(2 files, 1 obsolete file)
5.44 KB,
patch
|
jst
:
review+
mounir
:
checkin-
|
Details | Diff | Splinter Review |
4.33 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #591223 -
Flags: review?(jst)
Comment 1•12 years ago
|
||
Comment on attachment 591223 [details] [diff] [review] Patch - screen->mDocShell = aWindow->GetDocShell(); + screen->mIsChrome = IsChromeType(aWindow->GetDocShell); r=jst if you make that line actually compile :) The rest looks good!
Attachment #591223 -
Flags: review?(jst) → review+
Assignee | ||
Comment 2•12 years ago
|
||
Comment on attachment 591223 [details] [diff] [review] Patch Pushed with a compiling patch :)
Attachment #591223 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite-
Target Milestone: --- → mozilla13
Comment 3•12 years ago
|
||
Backed out of inbound since it conflicted with the prior push, which failed to build on all platforms: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4a763183482f https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c7eaaeec91
Target Milestone: mozilla13 → ---
Comment 4•12 years ago
|
||
Relanded, but the push had to be backed out again for bustage: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1ca8d5a931ac https://hg.mozilla.org/integration/mozilla-inbound/rev/33ffd55f2dfe Please can this/the rest of the push be run on try before landing a third time :-)
Assignee | ||
Comment 5•12 years ago
|
||
Justin did some changes that broke that patch... So I guess he will be glad to review the changes I had to do :)
Attachment #605802 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•12 years ago
|
Attachment #591223 -
Flags: checkin+ → checkin-
Comment 6•12 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #5) > Justin did some changes that broke that patch... So I guess he will be glad > to review the changes I had to do :) Sure! But which changes broke you, exactly? The last time I touched nsScreen.cpp was Dec 5.
Comment 7•12 years ago
|
||
Maybe it's bug 729430 (which I reviewed)?
Assignee | ||
Comment 8•12 years ago
|
||
Yes, indeed. You did review that bug so you can obviously review this one :)
Comment 9•12 years ago
|
||
Comment on attachment 605802 [details] [diff] [review] Update to current tip > + if (!sAllowScreenEnabledProperty || !(mIsChrome || IsWhiteListed(GetOwner()))) { Would you mind changing this to if (!sAllowScreenEnabledProperty || IsWhiteListed()) and making IsWhiteListed a member function which calls GetOwner() and checks mIsChrome?
Attachment #605802 -
Flags: review?(justin.lebar+bug) → review-
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #605802 -
Attachment is obsolete: true
Attachment #605812 -
Flags: review?(justin.lebar+bug)
Comment 11•12 years ago
|
||
> + nsCOMPtr<nsIDocument> doc = do_GetInterface(GetOwner()->GetDocShell());
> + nsIPrincipal *principal = doc->NodePrincipal();
doc can't be null?
Updated•12 years ago
|
Attachment #605812 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #11) > > + nsCOMPtr<nsIDocument> doc = do_GetInterface(GetOwner()->GetDocShell()); > > + nsIPrincipal *principal = doc->NodePrincipal(); > > doc can't be null? The code I'm moving is assuming that. Did you review the first patch without checking? :)
Comment 13•12 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12) > (In reply to Justin Lebar [:jlebar] from comment #11) > > > + nsCOMPtr<nsIDocument> doc = do_GetInterface(GetOwner()->GetDocShell()); > > > + nsIPrincipal *principal = doc->NodePrincipal(); > > > > doc can't be null? > > The code I'm moving is assuming that. Did you review the first patch without > checking? :) So it would seem...
Assignee | ||
Comment 14•12 years ago
|
||
I've added a null-check because it doesn't seem obvious that |doc| can never be null. Even |GetDocShell()| isn't guaranteed to not return null and the QI isn't guaranteed to succeed. Maybe it will always work in practice but better to be in the safe side here.
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/38a71ae7e06d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 16•12 years ago
|
||
I backed this out along with all the other patches in the initial push. Something in the push regressed Ts on Android by 20% and I don't know which patch is to blame: https://hg.mozilla.org/mozilla-central/rev/e94edfdb1f5b Regression Ts increase 20.6% on Android 2.2 Mozilla-Inbound -------------------------------------------------------------- Previous: avg 2653.656 stddev 87.856 of 30 runs up to revision 1239bd0779a6 New : avg 3201.178 stddev 110.202 of 5 runs since revision 0d61a0d8dba4 Change : +547.522 (20.6% / z=6.232) Graph : http://mzl.la/zD3EWy
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/64c0ad726111
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•