Closed Bug 1407498 Opened 2 years ago Closed 2 years ago

Don't query loadingprincipal attribute in common case

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch. v3
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.
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch Patch. (obsolete) — — Splinter Review 
Baku, sorry for bothering you again, can you check if my patch addresses bz's comment?

Thanks
Attachment #8917307 - Flags: feedback?(amarchesini)
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+
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!
Attached patch Patch v2. (obsolete) — — Splinter Review
Attachment #8917307 - Attachment is obsolete: true
Attachment #8917693 - Flags: review?(amarchesini)
Attachment #8917693 - Flags: review?(amarchesini) → review+
Attached patch Patch. v3 — — Splinter Review 
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
https://hg.mozilla.org/mozilla-central/rev/bad2101d54e4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.