Closed Bug 1481238 Opened Last year Closed Last year

Create WebIDL interface for BrowsingContext

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: farre, Assigned: farre)

References

Details

Attachments

(2 files, 6 obsolete files)

No description provided.
Assignee: nobody → afarre
Priority: -- → P3
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 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-
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)
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+
Fixed style comments, r+ carrying over.
Attachment #9001706 - Attachment is obsolete: true
Attachment #9003458 - Flags: review+
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)
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+
Blocks: 1486719
Fixed review comments, carrying over r+.
Attachment #9003459 - Attachment is obsolete: true
Attachment #9004498 - Flags: review+
Keywords: checkin-needed
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
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/612ca23ddb81
https://hg.mozilla.org/mozilla-central/rev/b5c403ab80f9
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Duplicate of this bug: 1467208
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.