Closed
Bug 1407498
Opened 8 years ago
Closed 8 years ago
Don't query loadingprincipal attribute in common case
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: allstars.chh, Assigned: allstars.chh)
References
Details
Attachments
(1 file, 2 obsolete files)
29.68 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
I added querying loadingprincipal attribute back in bug 1376971.
However in the follow-up, BZ pointed out some problems in https://bugzilla.mozilla.org/show_bug.cgi?id=1405195#c7
We shouldn't do this for the common case, also we need to update the comments.
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•8 years ago
|
||
Baku, sorry for bothering you again, can you check if my patch addresses bz's comment?
Thanks
Attachment #8917307 -
Flags: feedback?(amarchesini)
Comment 2•8 years ago
|
||
Comment on attachment 8917307 [details] [diff] [review]
Patch.
Review of attachment 8917307 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/html/HTMLMediaElement.cpp
@@ +1203,1 @@
> aElement->mLoadingSrcTriggeringPrincipal,
strange indentation.
Attachment #8917307 -
Flags: feedback?(amarchesini) → feedback+
![]() |
||
Comment 3•8 years ago
|
||
Some comments:
1) The code is mixing loadingPrincipal/triggeringPrincipal naming. Should probably be consistent.
2) "aLoadingNode->OwnerDoc()->NodePrincipal()" could just be "aLoadingNode->NodePrincipal()".
3) The "MOZ_ASSERT(nsContentUtils::IsSystemPrincipal(aLoadingNode->NodePrincipal())" is now
clearly redundant, because we already made that exact check earlier in the function.
4) We should probably rename the attribute to "triggeringprincipal" or so.
Thank you for cleaning this up!
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8917307 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8917693 -
Flags: review?(amarchesini)
Updated•8 years ago
|
Attachment #8917693 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 5•8 years ago
|
||
rebase after bug 1376973.
Attachment #8917693 -
Attachment is obsolete: true
Attachment #8921295 -
Flags: review+
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad2101d54e4
Don't query loadingprincipal in common case. r=baku
![]() |
||
Comment 7•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•