Closed
Bug 1233899
Opened 8 years ago
Closed 8 years ago
fix the feeds converter to use default user context origin attributes
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: huseby, Assigned: huseby)
References
Details
(Whiteboard: [userContextId][OA][uplift49-])
Attachments
(1 file, 5 obsolete files)
1.41 KB,
patch
|
huseby
:
review+
|
Details | Diff | Splinter Review |
This bug covers fixing the call to createCodebasePrincipal in browser/components/feeds/FeedConverter.js to use a default user context origin attribute.
Assignee | ||
Comment 1•8 years ago
|
||
this forces the feed converter to use a default user context origin attribute when it generates the about:feeds page since we're not going to isolate subscribed feeds by user context.
Attachment #8700249 -
Flags: review?(jonas)
Assignee | ||
Comment 2•8 years ago
|
||
The loadInfo.originAttributes used when creating the channel for loading the feed document may also need to have the default user context set in its origin attributes for caching reasons.
Assignee | ||
Updated•8 years ago
|
Component: Security → DOM
Product: Firefox → Core
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId]
Assignee | ||
Comment 3•8 years ago
|
||
rebased and updated
Attachment #8700249 -
Attachment is obsolete: true
Attachment #8700249 -
Flags: review?(jonas)
Attachment #8709212 -
Flags: review?(jonas)
Assignee | ||
Updated•8 years ago
|
Attachment #8709212 -
Flags: review?(jonas)
Assignee | ||
Comment 4•8 years ago
|
||
OK, so I dug deeper into this to better understand what is happening and have decided that the solution is to pass the origin attributes along with the default user context id since it is loading the about:feeds page. We decided that about:feeds would not be isolated by user context for now since the about:feeds page is the only UI for managing feed subscriptions. Until changes so that it can show the subscriptions broken out into each user context, we'll just force all feed subscriptions to be associated with the default user context.
Attachment #8709212 -
Attachment is obsolete: true
Attachment #8713820 -
Flags: review?(jonas)
Assignee | ||
Updated•8 years ago
|
Attachment #8713820 -
Flags: review?(jonas)
Assignee | ||
Comment 5•8 years ago
|
||
updated to new API
Attachment #8713820 -
Attachment is obsolete: true
Attachment #8722048 -
Flags: review?(jonas)
Comment on attachment 8722048 [details] [diff] [review] Bug_1233899.patch Review of attachment 8722048 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/feeds/FeedConverter.js @@ +255,5 @@ > let aboutFeedsURI = ios.newURI("about:feeds", null, null); > chromeChannel = ios.newChannelFromURIWithLoadInfo(aboutFeedsURI, loadInfo); > chromeChannel.originalURI = result.uri; > + let attrs = ChromeUtils.originAttributesFromDict(loadInfo.originAttributes); > + attrs.userContextId = 0 Based on our previous conversation, I suspect this isn't needed. And possibly not even desired. If this code is used to render the UI when a user opens a feed inside a tab, then likely we should honor user contexts in that tab. If this code is used to parse feeds for use in live bookmarks, then loadInfo.originAttributes.userContextId should already be 0.
Comment on attachment 8722048 [details] [diff] [review] Bug_1233899.patch Review of attachment 8722048 [details] [diff] [review]: ----------------------------------------------------------------- Removing review request given previous comment.
Attachment #8722048 -
Flags: review?(jonas)
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId] → [userContextId][OA]
Assignee | ||
Comment 8•8 years ago
|
||
We just need to do what comment 6 says and figure out if the code is used for the UI or if it is used in live bookmarks. Once we know that, we will know if it is necessary or not.
Assignee | ||
Updated•8 years ago
|
Assignee: huseby → nobody
Status: ASSIGNED → NEW
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → huseby
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Priority: -- → P1
Updated•8 years ago
|
Iteration: --- → 49.2 - May 23
Assignee | ||
Comment 9•8 years ago
|
||
The FeedConverter code that creates a principal does so when we convert the loaded feed into a result that can be loaded in the Feed rendering page. We want to carry the origin attributes from the channel that loaded the original feed document so that the feed rendering page is using a principal with the same origin attributes and therefore caches.
Attachment #8722048 -
Attachment is obsolete: true
Attachment #8760037 -
Flags: review?(amarchesini)
Assignee | ||
Comment 10•8 years ago
|
||
try push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe257d743195
Comment 11•8 years ago
|
||
Comment on attachment 8760037 [details] [diff] [review] Bug_1233899.patch Review of attachment 8760037 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/feeds/FeedConverter.js @@ +260,3 @@ > chromeChannel.owner = > + Services.scriptSecurityManager.createCodebasePrincipal(aboutFeedsURI, > + loadInfo.originAttributes); indentation.
Attachment #8760037 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 12•8 years ago
|
||
already r+ by :baku, just fixing the indentation mistake he caught.
Attachment #8760037 -
Attachment is obsolete: true
Attachment #8760449 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e052220dd9e fix the feeds converter to use default user context origin attributes. r=baku
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e052220dd9e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•8 years ago
|
Whiteboard: [userContextId][OA] → [userContextId][OA][uplift49?]
Updated•8 years ago
|
Whiteboard: [userContextId][OA][uplift49?] → [userContextId][OA][uplift49-]
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
•