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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

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

References

Details

Attachments

(1 file, 2 obsolete files)

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. v3Splinter 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
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.