Closed
Bug 1481238
Opened 6 years ago
Closed 6 years ago
Create WebIDL interface for BrowsingContext
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
Tracking
()
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: farre, Assigned: farre)
References
Details
Attachments
(2 files, 6 obsolete files)
4.78 KB,
patch
|
farre
:
review+
|
Details | Diff | Splinter Review |
13.42 KB,
patch
|
farre
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → afarre
Blocks: browsingcontext
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8997926 -
Flags: review?(peterv)
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•6 years ago
|
||
Fixing some issues after IRC discussion. Also, remember to include all code when creating patches. A side note: with Bug 1480113, the WebIDL+wrapper cache should probably move to live on ChromeBrowsingContext, and not on regular BrowsingContext.
Attachment #8997926 -
Attachment is obsolete: true
Attachment #8997926 -
Flags: review?(peterv)
Attachment #8998511 -
Flags: review?(peterv)
Comment 3•6 years ago
|
||
Comment on attachment 8998511 [details] [diff] [review] 0001-Bug-1481238-Create-WebIDL-interface-for-BrowsingCont.patch Review of attachment 8998511 [details] [diff] [review]: ----------------------------------------------------------------- I discussed this with nika yesterday, we actually think it makes more sense to have a |sequence<BrowsingContext> children| instead of firstChild/nextSibling.
Attachment #8998511 -
Flags: review?(peterv) → review-
Assignee | ||
Comment 4•6 years ago
|
||
A bunch of changes: * Moved to chrome-webidl * Changed to getChildren form firstChild, nextSibling * Changed parent to parent? * Exposed id
Attachment #8998511 -
Attachment is obsolete: true
Attachment #9001706 -
Flags: review?(peterv)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9001707 -
Flags: review?(peterv)
Comment 6•6 years ago
|
||
Comment on attachment 9001706 [details] [diff] [review] 0001-Bug-1481238-Create-WebIDL-interface-for-BrowsingCont.patch Review of attachment 9001706 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/BrowsingContext.cpp @@ +240,5 @@ > +} > + > +/* static */ void > +BrowsingContext::GetRootBrowsingContexts( > + nsTArray<RefPtr<BrowsingContext>>& aBrowsingContexts) BrowsingContext::GetRootBrowsingContexts(nsTArray<RefPtr<BrowsingContext>>& aBrowsingContexts) is not too long imho. ::: docshell/base/nsDocShell.cpp @@ +14337,5 @@ > return browsingContext.forget(); > } > > +NS_IMETHODIMP > +nsDocShell::GetBrowsingContext(BrowsingContext** aBrowsingContext) { I think generally we put the opening brace on its own line in this file. ::: dom/base/ChromeUtils.cpp @@ +782,5 @@ > > +/* static */ void > +ChromeUtils::GetRootBrowsingContexts( > + GlobalObject& aGlobal, > + nsTArray<RefPtr<BrowsingContext>>& aBrowsingContexts) You should be able to do: ChromeUtils::GetRootBrowsingContexts(GlobalObject& aGlobal, nsTArray<RefPtr<BrowsingContext>>& aBrowsingContexts) and stay below the 99 character limit. ::: dom/base/ChromeUtils.h @@ +188,5 @@ > > static already_AddRefed<Promise> > RequestIOActivity(GlobalObject& aGlobal, ErrorResult& aRv); > + > + static void GetRootBrowsingContexts( Please follow the style of the file, so: static void GetRootBrowsingContexts(GlobalObject& aGlobal, nsTArray<RefPtr<BrowsingContext>>& aBrowsingContexts);
Attachment #9001706 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 7•6 years ago
|
||
Fixed style comments, r+ carrying over.
Attachment #9001706 -
Attachment is obsolete: true
Attachment #9003458 -
Flags: review+
Assignee | ||
Comment 8•6 years ago
|
||
Switched to using Array.from instead of [].slice.call as discussed on IRC. Still r?
Attachment #9001707 -
Attachment is obsolete: true
Attachment #9001707 -
Flags: review?(peterv)
Attachment #9003459 -
Flags: review?(peterv)
Assignee | ||
Comment 9•6 years ago
|
||
Try is looking ok https://treeherder.mozilla.org/#/jobs?repo=try&revision=31bddb967131e05a207322df55f26d5bdcedacea&group_state=expanded
Comment 10•6 years ago
|
||
Comment on attachment 9003459 [details] [diff] [review] 0002-Bug-1481238-Add-tests-for-BrowsingContext.-r-peterv.patch Review of attachment 9003459 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/browser/browser_browsingContext.js @@ +27,5 @@ > + return contextId; > + }); > +} > + > +async function addFrame(browser, id, parentId, url) { You don't seems to use url in the tests, so maybe remove it. @@ +76,5 @@ > + } > + }); > +} > + > +function contains(id) { Hmm, this seems to return the context, so maybe name it getBrowsingContextById? @@ +79,5 @@ > + > +function contains(id) { > + let contexts = ChromeUtils.getRootBrowsingContexts(); > + > + while(contexts.length) { Space after while. @@ +117,5 @@ > + await removeFrame(browser, "frame2"); > + > + is(browsingContext1.getChildren().indexOf(browsingContext2), -1); > + > + // TODO(farre): I'm not sure how we should handle this. Can you file a bug about figuring out what we want parent to be?
Attachment #9003459 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 11•6 years ago
|
||
Fixed review comments, carrying over r+.
Attachment #9003459 -
Attachment is obsolete: true
Attachment #9004498 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 12•6 years ago
|
||
Hi, I was given this error when trying to apply your patch in order to land it: hg qpush -a applying 0001-Bug-1481238-Create-WebIDL-interface-for-BrowsingCont.patch patching file dom/chrome-webidl/moz.build Hunk #1 FAILED at 24 1 out of 1 hunks FAILED -- saving rejects to file dom/chrome-webidl/moz.build.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh 0001-Bug-1481238-Create-WebIDL-interface-for-BrowsingCont.patch Can you please take a look?
Flags: needinfo?(afarre)
Keywords: checkin-needed
Assignee | ||
Comment 13•6 years ago
|
||
Rebased patch to see if that fixes checkin. I'm using git, could that be the problem?
Attachment #9003458 -
Attachment is obsolete: true
Flags: needinfo?(afarre)
Attachment #9004825 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 14•6 years ago
|
||
Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/612ca23ddb81 Add tests for BrowsingContext. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c403ab80f9 Create WebIDL interface for BrowsingContext. r=peterv
Keywords: checkin-needed
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/612ca23ddb81 https://hg.mozilla.org/mozilla-central/rev/b5c403ab80f9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Comment 17•4 years ago
|
||
Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
Fission Milestone: --- → M4
You need to log in
before you can comment on or make changes to this bug.
Description
•