Closed Bug 1079725 Opened 10 years ago Closed 10 years ago

Add a BaseInContentPage class

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

defect

Tracking

(firefox33 unaffected, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 unaffected)

RESOLVED FIXED
Tracking Status
firefox33 --- unaffected
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: danisielm, Assigned: teodruta)

References

()

Details

(Whiteboard: [lib][lang=js][sprint])

Attachments

(2 files, 17 obsolete files)

712 bytes, text/plain
whimboo
: feedback+
Details
7.51 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
We started to discuss about having a BaseInContentPage class for all the in-content pages we have.

Currently from what I know we have:
- Add-ons page (already implemented) 
- Preferences page (getting implemented in bug 1005811)
- About Accounts page (bug 1079717)
- Security in-content pages (bug 863139)

Things could be in common:
- the dtds parameter and the getter
- the getElement() method
- the presence of getElements() method which will be different to all inherited classes
- an index of the tab where the page is open
- a close method
- ...

I'm not really pro about this idea of creating another abstract class because usually all this in-content pages are really different by their implementation. But we should first discuss about pros and cons.
In our tests we add an abstraction layer because we want that the tests are not created that tight to any implementation in Firefox. We want to have consistency, and should offer that to anyone who wants to create tests. If each and every in-content class looks different, we produce clutter no-one can easily remember.
Attached file structure.txt
I really want to get this in fast as we're in proccess of creating 3 libraries now which handles 3 in-content pages (see Comment 0)

Please see the attachement for the structure I propose.
I also have some questions besides the structure:

1. Is it duable that each instance of InContentPage class should be assigned to only one tab where the page is open ? Or it should handle all the tabs where the page with the specific "url" is open ?

2. For any case above, we are going to use the tabBrowser. Giving that the tabs library is under firefox/lib/, where should the base-in-content-page.js file live ?
Attachment #8502314 - Flags: feedback?(hskupin)
Comment on attachment 8502314 [details]
structure.txt

>function BaseInContentPage(aController) {
>  // assert.ok aController
>  _tabBrowser
>  _controller

I think we should use the BrowserWindow as parameter here. Then we should have access to browser.tabs or?

>  _dtds
>  _index // index of the tab where the page is
>  _url

Do we really need the index here? We are able to get this info via browser.tabs and don't have to worry about an outdated value of this property. The URL might be hard-coded for each subclass, right?

>BaseInContentPage.prototype = {
>  get controller,
>  get dtds,
>  get index,
>  get url,
>  get isOpen, // use tabs.getTabsWithURL(this._url)
>              // return true if this._index is in the above list of tabs
>  getElement, // the general function we are using everywhere
>  getElements, // in the base class will just return [];
>  close // close the tab (tabBrowser.close), using middleclick and _index as parameter

I miss the open() method.

(In reply to daniel.gherasim from comment #2)
> 1. Is it duable that each instance of InContentPage class should be assigned
> to only one tab where the page is open ? Or it should handle all the tabs
> where the page with the specific "url" is open ?

With the add-on manager we only handle the first tab found. If we will have the need to handle more, we can implement this later. But I doubt that we ever will need that.

> 2. For any case above, we are going to use the tabBrowser. Giving that the
> tabs library is under firefox/lib/, where should the base-in-content-page.js
> file live ?

Thunderbird also used the add-on manager in-content but as long as we cannot proof that our classes work there, we have to keep it under firefox/lib. The tabbrowser is btw very application specific (totally different implementations between FX, TB, and SM), so it would persist under firefox.
Attachment #8502314 - Flags: feedback?(hskupin) → feedback+
Depends on: 1081014
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
(In reply to Henrik Skupin (:whimboo) from comment #3)
> Comment on attachment 8502314 [details]
> structure.txt
> 
> >function BaseInContentPage(aController) {
> >  // assert.ok aController
> >  _tabBrowser
> >  _controller
> 
> I think we should use the BrowserWindow as parameter here. Then we should
> have access to browser.tabs or?

So you say we add this._tabs = new tabs.tabBrowser(this._controller) in the BrowserWindow class ?

> 
> >  _dtds
> >  _index // index of the tab where the page is
> 
> Do we really need the index here? We are able to get this info via
> browser.tabs and don't have to worry about an outdated value of this
> property. 

You're right, we can ommit it.

> >  _url
> The URL might be hard-coded for each subclass, right?

Correct!

> >BaseInContentPage.prototype = {
> >  get controller,
> >  get dtds,
> >  get index,
> >  get url,
> >  get isOpen, // use tabs.getTabsWithURL(this._url)
> >              // return true if this._index is in the above list of tabs
> >  getElement, // the general function we are using everywhere
> >  getElements, // in the base class will just return [];
> >  close // close the tab (tabBrowser.close), using middleclick and _index as parameter
> 
> I miss the open() method.
> 

Would be tricky to implement at this level.
Maybe here just a base methot, something in the lines like this one:

BaseInContentPage.prototype.open = function BICP_open(aCallback) {
  tabs.openNewTab();
  this._controller.open(this._url);
  this._controller.waitForPageLoad();
}
(In reply to daniel.gherasim from comment #4)
> > I think we should use the BrowserWindow as parameter here. Then we should
> > have access to browser.tabs or?
> 
> So you say we add this._tabs = new tabs.tabBrowser(this._controller) in the
> BrowserWindow class ?

If that doesn't exist yet we should do it. With that we would never have to require the tabs module separately, and lot of code can be safed. The only thing we should check if we can make this lazily loaded.

> > I miss the open() method.
> > 
> Would be tricky to implement at this level.
> Maybe here just a base methot, something in the lines like this one:
> 
> BaseInContentPage.prototype.open = function BICP_open(aCallback) {
>   tabs.openNewTab();
>   this._controller.open(this._url);
>   this._controller.waitForPageLoad();
> }

So what's necessary for the method to know? It needs the URL so it can check for the tab to be opened. This it can retrieve from this._url. It also has to wait for the tab selected / opened. Specific in-content pages can then implement e.g. selecting / waiting for a specific category.
Depends on: 1081851
Attached patch v1.patch (obsolete) — Splinter Review
First version of the patch.
Open method was constructed so it can be called from an inherited object if it will need to wait for the same events (TabSelect or TabOpen).
Attachment #8504588 - Flags: review?(andrei.eftimie)
Attachment #8504588 - Flags: review?(andreea.matei)
Whiteboard: [lib][lang=js] → [lib][lang=js][Blocked by bug 1081014][Blocked by bug 1081851]
Whiteboard: [lib][lang=js][Blocked by bug 1081014][Blocked by bug 1081851] → [lib][lang=js][Blocked by bug 1081014][Blocked by bug 1081851][sprint]
Comment on attachment 8504588 [details] [diff] [review]
v1.patch

Review of attachment 8504588 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good. Still need to have a dependency landed.
Only a few small issues for me.

::: firefox/lib/tests/testBaseInContentPage.js
@@ +9,5 @@
> +var baseICPage = require("../ui/base-in-content-page");
> +
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();
> +

nit: remove the empty line please

@@ +13,5 @@
> +
> +}
> +
> +function teardownModule(aModule) {
> +

Wondering if we should add a closeAllTabs method call here.

::: firefox/lib/ui/base-in-content-page.js
@@ +11,5 @@
> + * @constructor
> + *
> + * @param {MozMillController} aBrowserWindow
> + *        The browser window where the page lives
> + * @param {string} [aUrl]

Add the default value here

@@ +12,5 @@
> + *
> + * @param {MozMillController} aBrowserWindow
> + *        The browser window where the page lives
> + * @param {string} [aUrl]
> + *        Url of the page

URL

@@ +105,5 @@
> +   */
> +  close: function BaseInContentPage_close() {
> +    var pageIndex = tabs.getTabsWithURL(this._url, true)[0].index;
> +
> +    this._browserWindow.tabs.closeTab("middleClick", pageIndex);

Closing methods are probably not going to differ between different pages.
So we should support different closing methods... so lets add the possibility to add methods and pass them over to the closeTab method
Attachment #8504588 - Flags: review?(andrei.eftimie)
Attachment #8504588 - Flags: review?(andreea.matei)
Attachment #8504588 - Flags: review-
(In reply to Andrei Eftimie from comment #7)
> Review of attachment 8504588 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +105,5 @@
> > +   */
> > +  close: function BaseInContentPage_close() {
> > +    var pageIndex = tabs.getTabsWithURL(this._url, true)[0].index;
> > +
> > +    this._browserWindow.tabs.closeTab("middleClick", pageIndex);
> 
> Closing methods are probably not going to differ between different pages.
> So we should support different closing methods... so lets add the
> possibility to add methods and pass them over to the closeTab method

Hmm, index is supported only for the middleClick method, for the other methos we should do here this._browserWindow.tabs.selectedIndex = pageIndex before calling closeTab, so we are not closing the wrong tab, is that ok ?
(In reply to daniel.gherasim from comment #8)
> Hmm, index is supported only for the middleClick method, for the other
> methos we should do here this._browserWindow.tabs.selectedIndex = pageIndex
> before calling closeTab, so we are not closing the wrong tab, is that ok ?

Indeed. Yes, that sounds good to me.
Attached patch v2.patch (obsolete) — Splinter Review
Thanks, updated!
Attachment #8504588 - Attachment is obsolete: true
Attachment #8506039 - Flags: review?(andrei.eftimie)
Attachment #8506039 - Flags: review?(andreea.matei)
Attached patch v2.1.patch (obsolete) — Splinter Review
Attachment #8506039 - Attachment is obsolete: true
Attachment #8506039 - Flags: review?(andrei.eftimie)
Attachment #8506039 - Flags: review?(andreea.matei)
Attachment #8506042 - Flags: review?(andrei.eftimie)
Attachment #8506042 - Flags: review?(andreea.matei)
Whiteboard: [lib][lang=js][Blocked by bug 1081014][Blocked by bug 1081851][sprint] → [lib][lang=js][Blocked by bug 1081014][sprint]
Comment on attachment 8506042 [details] [diff] [review]
v2.1.patch

Review of attachment 8506042 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #8506042 - Flags: review?(hskupin)
Attachment #8506042 - Flags: review?(andrei.eftimie)
Attachment #8506042 - Flags: review?(andreea.matei)
Attachment #8506042 - Flags: review+
Comment on attachment 8506042 [details] [diff] [review]
v2.1.patch

Review of attachment 8506042 [details] [diff] [review]:
-----------------------------------------------------------------

Nice implementation for the base class. There are some items left to do or unclear. Lets figure that out so that we may be able to land all that before the weekend. Make sure to also update the name for Andreea in the commit message.

::: firefox/lib/tests/testBaseInContentPage.js
@@ +16,5 @@
> +  aModule.browserWindow.tabs.closeAllTabs();
> +}
> +
> +function testBaseInContentPage() {
> +  var page = new baseICPage.BaseInContentPage(browserWindow, "about:preferences");

You may wanna put this into setupModule.

@@ +19,5 @@
> +function testBaseInContentPage() {
> +  var page = new baseICPage.BaseInContentPage(browserWindow, "about:preferences");
> +  page.open();
> +
> +  assert.ok(page.isOpen, "Page has been found");

nit: We should better say that the tab with the incontent page has been opened.

::: firefox/lib/ui/base-in-content-page.js
@@ +12,5 @@
> + *
> + * @param {MozMillController} aBrowserWindow
> + *        The browser window where the page lives
> + * @param {string} [aUrl=""]
> + *        URL of the page

Hm, I wouldn't pass the URL in via the constructor, but let it specify by the subclass itself similar to the DTD entries.

@@ +103,5 @@
> +  /**
> +   * Close the current page (tab)
> +   *
> +   * @param {string} [aMethod]
> +   *        Method to use when closing

nit: Please note possible values or do we only want to allow callbacks similar to the BaseWindow class?

@@ +105,5 @@
> +   *
> +   * @param {string} [aMethod]
> +   *        Method to use when closing
> +   */
> +  close: function BaseInContentPage_close(aMethod) {

nit: you want to shorten the internal prefix to e.g. BICP.

@@ +115,5 @@
> +  /**
> +   * Open the page
> +   *
> +   * @param {function} [aCallback]
> +   *        Callback function to use for opening

nit: you can remove 'function'. That's implicit by calling it callback.

@@ +125,5 @@
> +        // Bug 1071566
> +        // Add new method in the the tabBrowser class to handle the TabSelect event
> +        // Replace code with the method which will get the callback as a parameter
> +        assert.waitFor(() => (tabBrowser.selectedIndex === prefTabs[0].index),
> +                       "The tab with index '" + prefTabs[0].index + "' has been selected");

I may miss something, but where is prefTabs coming from?

@@ +140,5 @@
> +      }
> +      else {
> +        this._browserWindow.tabs.openTab();
> +        this._browserWindow.controller.open(this._url);
> +        this._browserWindow.controller.waitForPageLoad();

Not sure if I'm sold with that option. In our tests we want to really test the reaction of Firefox when triggering actions. For me this feels like a unit test behavior, which we most likely don't want.

When do you think this makes it useful?

@@ +144,5 @@
> +        this._browserWindow.controller.waitForPageLoad();
> +      }
> +    }
> +  }
> +}

nit: missing semicolon.
Attachment #8506042 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 8506042 [details] [diff] [review]
> v2.1.patch
>
> Review of attachment 8506042 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: firefox/lib/ui/base-in-content-page.js
> @@ +12,5 @@
> > + *
> > + * @param {MozMillController} aBrowserWindow
> > + *        The browser window where the page lives
> > + * @param {string} [aUrl=""]
> > + *        URL of the page
>
> Hm, I wouldn't pass the URL in via the constructor, but let it specify by
> the subclass itself similar to the DTD entries.
>

Sounds good. But I'm not sure how to test the class after that.
Maybe a setter for the url ?

Also while working on the about:accounts page I noticed we may want to use this.url instead this._url in our methods (open, isOpen, close etc.).

That's because we may want the url of the class to be dynamically generated (e.g. : about:accounts?entrypoint=menubar&action=signin - the query part). Not sure here how to proceed exactly here ... Do a similar implementation as ignoring fragments, also ignoring the query ? Or generate the url sth like :

AboutAccountsPage.prototype.__defineGetter__('url', function () {
  // construct the query using this._action and this._entryPoint
  return this._url + query;
});

For this case I will go with the first solution (ignoring query) given that if we're trying to open the page (using url with different query) then the same tab will be selected and just the query being changed.
Flags: needinfo?(hskupin)
> @@ +140,5 @@
> > +      }
> > +      else {
> > +        this._browserWindow.tabs.openTab();
> > +        this._browserWindow.controller.open(this._url);
> > +        this._browserWindow.controller.waitForPageLoad();
> 
> Not sure if I'm sold with that option. In our tests we want to really test
> the reaction of Firefox when triggering actions. For me this feels like a
> unit test behavior, which we most likely don't want.
> 
> When do you think this makes it useful?
> 

This is something only specific to a base in-content page.
e.g.: for a security warning page for which we don't have a specific action.
Attached patch v3.patch (obsolete) — Splinter Review
Thanks Henrik, created a patch with what I see it as the best solution, above stuff still remain to discuss and if we find another implementation I'll do it.

* made a setter for the url attribute;
* used this.url in the other functions in case we want to make it a dynamic parameter in a inherited class;
* refactored close method to use callback;
* extended the lib test a bit.
Attachment #8506042 - Attachment is obsolete: true
Attachment #8506764 - Flags: review?(andrei.eftimie)
Attachment #8506764 - Flags: review?(andreea.matei)
Comment on attachment 8506764 [details] [diff] [review]
v3.patch

Changing the flag to feedback from Henrik to see that's the implementation we want.
Attachment #8506764 - Flags: review?(andrei.eftimie)
Attachment #8506764 - Flags: review?(andreea.matei)
Attachment #8506764 - Flags: feedback?(hskupin)
(In reply to daniel.gherasim from comment #14)
> Sounds good. But I'm not sure how to test the class after that.
> Maybe a setter for the url ?

The URL is always the same, it will not change. So I don't see why we have to implement properties for it. And you are right in terms that you won't be able to test any aspect of this class. It should be done with subclasses.

> Also while working on the about:accounts page I noticed we may want to use
> this.url instead this._url in our methods (open, isOpen, close etc.).
> That's because we may want the url of the class to be dynamically generated
> (e.g. : about:accounts?entrypoint=menubar&action=signin - the query part).
> Not sure here how to proceed exactly here ... Do a similar implementation as
> ignoring fragments, also ignoring the query ? Or generate the url sth like :

So the URL should contain a unique identifier for this particular page. And this is 'about:XYZ'. This is used for querying if a similar tab has already been opened. The fragment we won't need.

On the other side we could have this.url but which returns the full URL, and in parallel have this.type or so with 'about:XYZ'. In some cases you might wanna get the full URL and that way you do not have to go over the controller.
Flags: needinfo?(hskupin)
(In reply to daniel.gherasim from comment #15)
> This is something only specific to a base in-content page.
> e.g.: for a security warning page for which we don't have a specific action.

Those page are about:error or? But we do not expose the URL and keep the baseURI in the locationbar. In which cases you want to use this method?
Comment on attachment 8506764 [details] [diff] [review]
v3.patch

Review of attachment 8506764 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my former comments on the bug, and the inline comments here.

::: firefox/lib/ui/base-in-content-page.js
@@ +113,5 @@
> +  /**
> +   * Close the current page (tab)
> +   *
> +   * @param {string} [aCallback]
> +   *        Define new function that closes the page

nit: Please use the same description as for all other open/close methods we have.

@@ +120,5 @@
> +    var pageIndex = tabs.getTabsWithURL(this.url, true)[0].index;
> +    this._browserWindow.tabs.selectedIndex = pageIndex;
> +
> +    if (typeof aCallback === "function") {
> +      aCallback(pageIndex);

Why do we have to pass in the pageIndex?

@@ +123,5 @@
> +    if (typeof aCallback === "function") {
> +      aCallback(pageIndex);
> +    }
> +    else {
> +      this._browserWindow.tabs.closeTab();

The jsdoc does not reflect that yet. You should add which default is being used.

@@ +142,5 @@
> +        // Add new method in the the tabBrowser class to handle the TabSelect event
> +        // Replace code with the method which will get the callback as a parameter
> +        var pageTabs = tabs.getTabsWithURL(this.url, true);
> +        assert.waitFor(() => (this._browserWindow.tabs.selectedIndex === pageTabs[0].index),
> +                       "The tab with index '" + pageTabs[0].index + "' has been selected");

Please move indexing [0] to line 144. There is no need to do this in each iteration.

@@ +147,5 @@
> +      }
> +      else {
> +        this._browserWindow.tabs.openTab({method: "callback", callback: aCallback});
> +        this._browserWindow.controller.waitForPageLoad();
> +        this._browserWindow.controller.sleep(100);

That sleep has to be removed. I don't see any value in having it in the base class.
Attachment #8506764 - Flags: feedback?(hskupin) → feedback+
(In reply to Henrik Skupin (:whimboo) from comment #19)
> (In reply to daniel.gherasim from comment #15)
> > This is something only specific to a base in-content page.
> > e.g.: for a security warning page for which we don't have a specific action.
> 
> Those page are about:error or? But we do not expose the URL and keep the
> baseURI in the locationbar. In which cases you want to use this method?

Hmm, if we want to handle the reported-web-forgey in-content page how would you suggest to open it ? I would use:

var errorPage = new ErrorPage(browserWindow);
errorPage.url = "https://www.itisatrap.org/firefox/its-a-trap.html";
errorPage.open(); // will open a new tab, access the page and wait for loading (from the base class) .
// also we want to test if "#errorPageContainer" element is shown in the open method.

That's the case we may need this method, so we would only call base.open. Otherwise we could write this code directly into the ErrorPage class.

(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to daniel.gherasim from comment #14)
> > Sounds good. But I'm not sure how to test the class after that.
> > Maybe a setter for the url ?
> 
> The URL is always the same, it will not change. So I don't see why we have
> to implement properties for it. And you are right in terms that you won't be
> able to test any aspect of this class. It should be done with subclasses.
> 

So no test for the base class ? 
How about setting the url for the above case related the security pages ?

> > Also while working on the about:accounts page I noticed we may want to use
> > this.url instead this._url in our methods (open, isOpen, close etc.).
> > That's because we may want the url of the class to be dynamically generated
> > (e.g. : about:accounts?entrypoint=menubar&action=signin - the query part).
> > Not sure here how to proceed exactly here ... Do a similar implementation as
> > ignoring fragments, also ignoring the query ? Or generate the url sth like :
> 
> So the URL should contain a unique identifier for this particular page. And
> this is 'about:XYZ'. This is used for querying if a similar tab has already
> been opened. The fragment we won't need.
> 
> On the other side we could have this.url but which returns the full URL, and
> in parallel have this.type or so with 'about:XYZ'. In some cases you might
> wanna get the full URL and that way you do not have to go over the
> controller.

Then we should improve the getTabsWithURL method to also accept ignoring the query part, right ?

For e.g.: we have about:accounts?entrypoint=menubar -> we access it from the menupanel -> the same tab will be selected but the url will be changed to : about:accounts?entrypoint=menupanel. Interesting is that if we access it from a different place, will also have to wait for the page to load. This will be a bit tricky to handle. Most likely we will completely overwrite the base open method in this class.
(In reply to daniel.gherasim from comment #21)
> var errorPage = new ErrorPage(browserWindow);
> errorPage.url = "https://www.itisatrap.org/firefox/its-a-trap.html";

As the DOM inspector tells me this is an in-content page of the type `about:blocked`:

about:blocked?e=phishingBlocked&u=https%3A//www.itisatrap.org/firefox/its-a-trap.html&s=blacklist&c=UTF-8&f=regular&d=The%20website%20at%20www.itisatrap.org%20has%20been%20reported%20as%20a%20web%20forgery%20designed%20to%20trick%20users%20into%20sharing%20personal%20or%20financial%20information.

So internally it has to represent `about:blocked`. Also shall we better pass in the document window but not the browser window? This might also affect our other in-content classes.

> errorPage.open(); // will open a new tab, access the page and wait for
> loading (from the base class) .

How would you cover the case when a new instance is necessary for an already open page? Not sure if we can always create an instance before and call open() to wait for it.

> // also we want to test if "#errorPageContainer" element is shown in the
> open method.

But that's related to about:error and not about:blocked, right?

> (In reply to Henrik Skupin (:whimboo) from comment #18)
> So no test for the base class ? 

If it's not possible you won't be able to create tests. The base class is something like an abstract class.

> How about setting the url for the above case related the security pages ?

Not sure I fully understand this question. Can you please re-phrase?

> Then we should improve the getTabsWithURL method to also accept ignoring the
> query part, right ?

That's what you did and which already have been landed? The about pages should always make use of the base URL without the query to call the above mentioned method.

> For e.g.: we have about:accounts?entrypoint=menubar -> we access it from the
> menupanel -> the same tab will be selected but the url will be changed to :
> about:accounts?entrypoint=menupanel. Interesting is that if we access it
> from a different place, will also have to wait for the page to load. This
> will be a bit tricky to handle. Most likely we will completely overwrite the
> base open method in this class.

Keep in mind that this is a remote page, which is getting loaded here.
Attached patch v4.patch (obsolete) — Splinter Review
Thanks for all the feedback Henrik! That was great.
I won't answer to your questions as you already answerd me in an indirect way.

So after a quick check on all of our current used or needed pages (by type: about:accounts, about:preferences, about:blocked, about:error, about:addons) I ended up with this patch.

Basically:
==========
* We use the contentWindow as the basic element of each page.
* Similar to the BaseWindow, initially the contentWindow can be null and instantiated afterwards in the open method.
* We get the current url using contentWindow.location.href.
* We allow type declaration for each page
! getTabs will usually be changed in the inherited classes, based on the functionality of that page
Attachment #8506764 - Attachment is obsolete: true
Attachment #8509403 - Flags: review?(andrei.eftimie)
Attachment #8509403 - Flags: review?(andreea.matei)
Comment on attachment 8509403 [details] [diff] [review]
v4.patch

Review of attachment 8509403 [details] [diff] [review]:
-----------------------------------------------------------------

Needs a small update in the manifest:
> patching file firefox/lib/tests/manifest.ini
> Hunk #1 FAILED at 0
> 1 out of 1 hunks FAILED -- saving rejects to file firefox/lib/tests/manifest.ini.rej

We're starting to make a lot of assumptions on the state of Firefox, when tests are (and should be) much more straightforward.
In a test it should be clear to us if a particular page is open or not.

Maybe we should focus more on making sure the state is properly cleaned up between tests.

::: firefox/lib/ui/base-in-content-page.js
@@ +109,5 @@
> +   * Get all the tabs containing this page's URL
> +   * This will most likely get overwritten in the inherited classes
> +   * depending on how we will search for the page state
> +   *
> +   * @returns {array[]} Array of tabs

[] denotes an array, you have to specify the type of each entry in the array

@@ +111,5 @@
> +   * depending on how we will search for the page state
> +   *
> +   * @returns {array[]} Array of tabs
> +   */
> +  getTabs: function BICP_getTabs() {

Could this work better as a getter?

@@ +123,5 @@
> +   * @param {function} [aCallback=this._browserWindow.tabs.closeTab()]
> +   *        Callback to close the page
> +   */
> +  close: function BICP_close(aCallback) {
> +    if (!this.contentWindow.closed) {

Could we change this to a positive test and return early if the window is already closed?

@@ +128,5 @@
> +      var pageIndex = this.getTabs()[0].index;
> +      this._browserWindow.tabs.selectedIndex = pageIndex;
> +
> +      aCallback = (typeof aCallback === "function") ? aCallback
> +                    : () => { this._browserWindow.tabs.closeTab(); }

Don't reassign a method argument please.

Also this is a bit hard to read. Please reindent, or re-engineer it.

@@ +129,5 @@
> +      this._browserWindow.tabs.selectedIndex = pageIndex;
> +
> +      aCallback = (typeof aCallback === "function") ? aCallback
> +                    : () => { this._browserWindow.tabs.closeTab(); }
> +      aCallback();

We should maybe also wait for the tab to close?

What happens if it is the only opened tab? Will we also close the window? In that case do we want something similar to tabs.closeAllTabs (open the default page instead)?
Attachment #8509403 - Flags: review?(andrei.eftimie)
Attachment #8509403 - Flags: review?(andreea.matei)
Attachment #8509403 - Flags: review-
Attached patch v4.1.patch (obsolete) — Splinter Review
Attachment #8509403 - Attachment is obsolete: true
Attachment #8512016 - Flags: review?(andrei.eftimie)
Attachment #8512016 - Flags: review?(andreea.matei)
Comment on attachment 8512016 [details] [diff] [review]
v4.1.patch

Review of attachment 8512016 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple things to check please.
Otherwise lgtm.

::: firefox/lib/tests/testBaseInContentPage.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Running this test on OSX against Nightly I see the following (which doesn't impact the test):
> A coding exception was thrown in a Promise resolution callback.
> See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
> Full message: TypeError: this.iframe.contentWindow is null
> Full stack: wrapper.injectData@chrome://browser/content/aboutaccounts/aboutaccounts.js:264:5
> wrapper.onSessionStatus/<@chrome://browser/content/aboutaccounts/aboutaccounts.js:212:24
> Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
> this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
> ...

Pleas make a quick check to be sure this isn't a problem from our code.

::: firefox/lib/ui/base-in-content-page.js
@@ +128,5 @@
> +  close: function BICP_close(aCallback) {
> +    if (this.contentWindow.closed) {
> +      return;
> +    }
> +

Please add a short comment here so its clear this is for the last opened tab.

We have this code as part of closeAllTabs() as well. I wonder whether this should be part of closeTab() instead.
Have an argument to closeTab() which defaults to false, and if true make sure to leave 1 default tab open instead of closing the window.

This would get this duplicated code out of here and of closeAllTabs() into closeTab().
Actually closeALlTabs() just ignores the last tab, and directly changes it's URL, which seems like a nice approach.

So while trying to close a tab, if its not the last one close it, if it's the last one open BROWSER_NEW_TAB_URL...
I guess this could be a followup

@@ +138,5 @@
> +
> +    var pageIndex = this.tabs[0].index;
> +    this._browserWindow.tabs.selectedIndex = pageIndex;
> +
> +    if (typeof aCallback === "function")

Please always include curly brackets.
Attachment #8512016 - Flags: review?(andrei.eftimie)
Attachment #8512016 - Flags: review?(andreea.matei)
Attachment #8512016 - Flags: review-
Attached patch v4.2.patch (obsolete) — Splinter Review
(In reply to Andrei Eftimie from comment #26)
> Comment on attachment 8512016 [details] [diff] [review]
> v4.1.patch
> 
> Review of attachment 8512016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a couple things to check please.
> Otherwise lgtm.
> 
> ::: firefox/lib/tests/testBaseInContentPage.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Running this test on OSX against Nightly I see the following (which doesn't
> impact the test):
> > A coding exception was thrown in a Promise resolution callback.
> > See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
> > Full message: TypeError: this.iframe.contentWindow is null
> > Full stack: wrapper.injectData@chrome://browser/content/aboutaccounts/aboutaccounts.js:264:5
> > wrapper.onSessionStatus/<@chrome://browser/content/aboutaccounts/aboutaccounts.js:212:24
> > Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
> > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
> > ...
> 
> Pleas make a quick check to be sure this isn't a problem from our code.
> 

Yeah, I also saw that on ubuntu.
That's because we close the firefox before the content in the frame is completely loaded.
But we should be safe in tests, given that we'll handle the content.

> We have this code as part of closeAllTabs() as well. I wonder whether this
> should be part of closeTab() instead.
> Have an argument to closeTab() which defaults to false, and if true make
> sure to leave 1 default tab open instead of closing the window.
> 
> This would get this duplicated code out of here and of closeAllTabs() into
> closeTab().
> Actually closeALlTabs() just ignores the last tab, and directly changes it's
> URL, which seems like a nice approach.
> 
> So while trying to close a tab, if its not the last one close it, if it's
> the last one open BROWSER_NEW_TAB_URL...
> I guess this could be a followup
> 

Follow up sounds great.
Attachment #8512016 - Attachment is obsolete: true
Attachment #8513410 - Flags: review?(hskupin)
Whiteboard: [lib][lang=js][Blocked by bug 1081014][sprint] → [lib][lang=js][sprint]
Comment on attachment 8513410 [details] [diff] [review]
v4.2.patch

Review of attachment 8513410 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/ui/base-in-content-page.js
@@ +7,5 @@
> +var tabs = require("../tabs");
> +var utils = require("../../../lib/utils");
> +
> +/**
> + * Base In-Content page class (abstract)

Please add @abstract to the appropriate methods

@@ +14,5 @@
> + * @param {object} aBrowserWindow
> + *        Browser window where the page lives
> + * @param {object} [aContentWindow=null]
> + *        Content of the page
> + *        initially 'null' if initialized later in a method (e.g.: open())

In which cases this parameter would not be null?

@@ +29,5 @@
> +BaseInContentPage.prototype = {
> +  /**
> +   * Returns the content window of the page
> +   *
> +   * @returns {object} The browser window instance

content window

@@ +58,5 @@
> +   * Get all the tabs containing this page's URL
> +   * This will most likely get overwritten in the inherited classes
> +   * depending on how we will search for the page state
> +   *
> +   * @returns {object[]} Array of tabs

Do we know the specific type of the tabs?

@@ +125,5 @@
> +   * @param {function} [aCallback=this._browserWindow.tabs.closeTab()]
> +   *        Callback to close the page
> +   */
> +  close: function BICP_close(aCallback) {
> +    if (this.contentWindow.closed) {

Please check first if contentWindow is not null.

@@ +129,5 @@
> +    if (this.contentWindow.closed) {
> +      return;
> +    }
> +
> +    // Handle the case where page is loaded in the last opened tab

Can you please explain in more detail why this code is necessary? I don't get it.

nit: when page...

@@ +132,5 @@
> +
> +    // Handle the case where page is loaded in the last opened tab
> +    if (this._browserWindow.tabs.length === 1) {
> +      var newTabUrl = this._browserWindow.controller.window.BROWSER_NEW_TAB_URL;
> +      this._browserWindow.controller.open(newTabUrl);

Lets try to use the getter where-ever you can.

@@ +137,5 @@
> +      this._browserWindow.controller.waitForPageLoad();
> +    }
> +
> +    var pageIndex = this.tabs[0].index;
> +    this._browserWindow.tabs.selectedIndex = pageIndex;

You can assign it directly.
Attachment #8513410 - Flags: review?(hskupin) → review-
Assignee: danisielm → teodor.druta
Attached patch v4.3.patch (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #28)

> 
> ::: firefox/lib/ui/base-in-content-page.js
> @@ +7,5 @@
> > +var tabs = require("../tabs");
> > +var utils = require("../../../lib/utils");
> > +
> > +/**
> > + * Base In-Content page class (abstract)
> 
> Please add @abstract to the appropriate methods
> 

I think the only abstract method we have is getElements() ?

> @@ +14,5 @@
> > + * @param {object} aBrowserWindow
> > + *        Browser window where the page lives
> > + * @param {object} [aContentWindow=null]
> > + *        Content of the page
> > + *        initially 'null' if initialized later in a method (e.g.: open())
> 
> In which cases this parameter would not be null?

contentWindow value is assigned in the open() method and we can't really set the contentWindow on class instantiation, I don't think we need aContentWindow parameter here as it will always be null on class instantiation, removed it.

> 
> @@ +58,5 @@
> > +   * Get all the tabs containing this page's URL
> > +   * This will most likely get overwritten in the inherited classes
> > +   * depending on how we will search for the page state
> > +   *
> > +   * @returns {object[]} Array of tabs
> 
> Do we know the specific type of the tabs?
> 

I think it's ElemBase ?

> 
> @@ +129,5 @@
> > +    if (this.contentWindow.closed) {
> > +      return;
> > +    }
> 
> Can you please explain in more detail why this code is necessary? I don't
> get it.
> 

I think Daniel wrote this code so if someone tries to call .close() method before opening a BICP it won't throw an error,
I don't think this is the correct behaviour added a throw error message.

> @@ +132,5 @@
> > +      var newTabUrl = this._browserWindow.controller.window.BROWSER_NEW_TAB_URL;
> > +      this._browserWindow.controller.open(newTabUrl);
> 

Fixed also small code rewrite.

> 
> @@ +137,5 @@
> > +    var pageIndex = this.tabs[0].index;
> > +    this._browserWindow.tabs.selectedIndex = pageIndex;
> 
> You can assign it directly.

Fixed.
Attachment #8514887 - Flags: review?(andrei.eftimie)
Attachment #8514887 - Flags: review?(andreea.matei)
Attachment #8513410 - Attachment is obsolete: true
Comment on attachment 8514887 [details] [diff] [review]
v4.3.patch

Review of attachment 8514887 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/ui/base-in-content-page.js
@@ +128,5 @@
> +   *        Callback to close the page
> +   */
> +  close: function BICP_close(aCallback) {
> +    if (!this.contentWindow || this.contentWindow.closed) {
> +      throw new Error("No content window found !")

For throwing errors use the assertion module:
> assert.fail(%message%)

Also don't forget about trailing semicolon

@@ +134,5 @@
> +
> +    // Handle the case when page is loaded in the last opened tab
> +    if (this._browserWindow.tabs.length === 1) {
> +      this._browserWindow.controller.open(this.URL);
> +      this._browserWindow.controller.waitForPageLoad();

I think Henrik was saying to have a getter for _browserWindow (similar to what we have for contentWindow). And use that instead of the "private" object.
Attachment #8514887 - Flags: review?(andrei.eftimie)
Attachment #8514887 - Flags: review?(andreea.matei)
Attachment #8514887 - Flags: review-
Attached patch v4.4.patch (obsolete) — Splinter Review
Changed throw new Error(%s) to assert.fail(%s).

Added a getter for baseWindow object.
Updated all the code from using the private method to the public one:
> this._baseWindow => this.baseWindow
Attachment #8514887 - Attachment is obsolete: true
Attachment #8515978 - Flags: review?(andrei.eftimie)
Attachment #8515978 - Flags: review?(andreea.matei)
Attachment #8515978 - Flags: review?(hskupin)
Attachment #8515978 - Flags: review?(andrei.eftimie)
Attachment #8515978 - Flags: review?(andreea.matei)
Attachment #8515978 - Flags: review+
Comment on attachment 8515978 [details] [diff] [review]
v4.4.patch

Review of attachment 8515978 [details] [diff] [review]:
-----------------------------------------------------------------

With the below mentioned changes you will get my r+.

::: firefox/lib/tests/testBaseInContentPage.js
@@ +18,5 @@
> +}
> +
> +function testBaseInContentPage() {
> +  page.open(() => {
> +    browserWindow.controller.mainMenu.click("#sync-setup");

Mind using the add-on manager here? It's more safe given that this would be a local chrome page, and not a remote website.

::: firefox/lib/ui/base-in-content-page.js
@@ +7,5 @@
> +var tabs = require("../tabs");
> +var utils = require("../../../lib/utils");
> +
> +/**
> + * Base In-Content page class (abstract)

nit: This should also be a new line with @abstract and not as part of the description.

@@ +12,5 @@
> + * @constructor
> + *
> + * @param {object} aBrowserWindow
> + *        Browser window where the page lives
> + * @param {object} [aContentWindow=null]

There is no such parameter anymore for the constructor.

@@ +82,5 @@
> +   * Get the URL of the page
> +   *
> +   * @returns {string} URL of the page
> +   */
> +  get URL() {

I don't think that we ever used allcaps for properties. This is usually reserved for constants. Do it like .dtds and make it lower-case letters.

@@ +144,5 @@
> +    // Handle the case when page is loaded in the last opened tab
> +    if (this.browserWindow.tabs.length === 1) {
> +      var newTabUrl = this.browserWindow.controller.window.BROWSER_NEW_TAB_URL;
> +      this.browserWindow.controller.open(newTabUrl);
> +      this.browserWindow.controller.waitForPageLoad();

I do not want to replicate what we already have in TabBrowser.closeAllTabs(). Please simply call this function in case a single tab is open only.
Attachment #8515978 - Flags: review?(hskupin) → review+
Attached patch v4.5.patch (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #32)
> 
> ::: firefox/lib/tests/testBaseInContentPage.js
> @@ +18,5 @@
> > +}
> > +
> > +function testBaseInContentPage() {
> > +  page.open(() => {
> > +    browserWindow.controller.mainMenu.click("#sync-setup");
> 
> Mind using the add-on manager here? It's more safe given that this would be
> a local chrome page, and not a remote website.
> 

Updated the code now waits for the add-on manager page to open.

> ::: firefox/lib/ui/base-in-content-page.js
> @@ +7,5 @@
> > +var tabs = require("../tabs");
> > +var utils = require("../../../lib/utils");
> > +
> > +/**
> > + * Base In-Content page class (abstract)
> 
> nit: This should also be a new line with @abstract and not as part of the
> description.
> 

Fixed.

> @@ +12,5 @@
> > + * @constructor
> > + *
> > + * @param {object} aBrowserWindow
> > + *        Browser window where the page lives
> > + * @param {object} [aContentWindow=null]
> 
> There is no such parameter anymore for the constructor.
> 

Removed.

> @@ +82,5 @@
> > +   * Get the URL of the page
> > +   *
> > +   * @returns {string} URL of the page
> > +   */
> > +  get URL() {
> 
> I don't think that we ever used allcaps for properties. This is usually
> reserved for constants. Do it like .dtds and make it lower-case letters.
> 

Fixed.

> @@ +144,5 @@
> > +    // Handle the case when page is loaded in the last opened tab
> > +    if (this.browserWindow.tabs.length === 1) {
> > +      var newTabUrl = this.browserWindow.controller.window.BROWSER_NEW_TAB_URL;
> > +      this.browserWindow.controller.open(newTabUrl);
> > +      this.browserWindow.controller.waitForPageLoad();
> 
> I do not want to replicate what we already have in
> TabBrowser.closeAllTabs(). Please simply call this function in case a single
> tab is open only.

Modified to use closeAllTabs().
Attachment #8515978 - Attachment is obsolete: true
Attachment #8517465 - Flags: review?(andrei.eftimie)
Comment on attachment 8517465 [details] [diff] [review]
v4.5.patch

Review of attachment 8517465 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the mentioned change

::: firefox/lib/ui/base-in-content-page.js
@@ +141,5 @@
> +
> +    // Handle the case when page is loaded in the last opened tab
> +    if (this.browserWindow.tabs.length === 1) {
> +      this.browserWindow.tabs.closeAllTabs();
> +    }

We'll also have to return here, otherwise we'll end up trying to close the last opened tab below, in this case also `aCallback` doesn't has any effect, not sure how desirable that is.
Attachment #8517465 - Flags: review?(andrei.eftimie) → review+
Attached patch v4.6.patch (obsolete) — Splinter Review
Changes in the close() method of the BaseInContentPage class and small addition to testCaseContentPage.js.
Attachment #8517465 - Attachment is obsolete: true
Attachment #8518144 - Flags: review?(andrei.eftimie)
Comment on attachment 8518144 [details] [diff] [review]
v4.6.patch

Review of attachment 8518144 [details] [diff] [review]:
-----------------------------------------------------------------

r=me is actually a r+ with those changes implemented. So moving this to Henrik.
Attachment #8518144 - Flags: review?(hskupin)
Attachment #8518144 - Flags: review?(andrei.eftimie)
Attachment #8518144 - Flags: review+
Comment on attachment 8518144 [details] [diff] [review]
v4.6.patch

Review of attachment 8518144 [details] [diff] [review]:
-----------------------------------------------------------------

Actually Henrik also r+ this in comment 32 with the changes implemented. Have a little comment below, add a checkin flag with this fixed so we can land it.

::: firefox/lib/ui/base-in-content-page.js
@@ +147,5 @@
> +    this.browserWindow.tabs.selectedIndex = this.tabs[0].index;
> +
> +    var close = (typeof aCallback === "function") ? aCallback :
> +                                                    () => {
> +                                                      this.browserWindow.tabs.closeTab();

Please leave this as it was. It looks odd now due to the indentation.
Attachment #8518144 - Flags: review?(hskupin)
Attached patch v4.6.1.patch (obsolete) — Splinter Review
Reverted changes back for the if statement in the close() method of BaseInContentPage class.
Attachment #8518144 - Attachment is obsolete: true
Attachment #8518234 - Flags: review?(andrei.eftimie)
Attachment #8518234 - Flags: review?(andreea.matei)
Comment on attachment 8518234 [details] [diff] [review]
v4.6.1.patch

Review of attachment 8518234 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tests/manifest.ini
@@ +1,5 @@
>  [DEFAULT]
>  server-root = ../../../data
>  
> +[testBaseInContentPage.js]
> +[testBrowserWindow.js]
\ No newline at end of file

Why have you removed all library tests?

::: firefox/lib/ui/base-in-content-page.js
@@ +140,5 @@
> +    }
> +
> +    // Handle the case when page is loaded in the last opened tab
> +    if (this.browserWindow.tabs.length === 1) {
> +       this.browserWindow.tabs.openTab(this.browserWindow.controller.window.BROWSER_NEW_TAB_URL);

Please split this into 2 lines (best to assign the NEW_TAB_URL value to a variable), and mind the indentation.
Attachment #8518234 - Flags: review?(andrei.eftimie)
Attachment #8518234 - Flags: review?(andreea.matei)
Attachment #8518234 - Flags: review-
Comment on attachment 8518234 [details] [diff] [review]
v4.6.1.patch

Review of attachment 8518234 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/ui/base-in-content-page.js
@@ +140,5 @@
> +    }
> +
> +    // Handle the case when page is loaded in the last opened tab
> +    if (this.browserWindow.tabs.length === 1) {
> +       this.browserWindow.tabs.openTab(this.browserWindow.controller.window.BROWSER_NEW_TAB_URL);

I do not understand this line at all now. What happened with my request for closeAllTabs()? Why are we opening a new tab here?
(In reply to Henrik Skupin (:whimboo) from comment #40)
> Comment on attachment 8518234 [details] [diff] [review]
> v4.6.1.patch
> 
> Review of attachment 8518234 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/ui/base-in-content-page.js
> @@ +140,5 @@
> > +    }
> > +
> > +    // Handle the case when page is loaded in the last opened tab
> > +    if (this.browserWindow.tabs.length === 1) {
> > +       this.browserWindow.tabs.openTab(this.browserWindow.controller.window.BROWSER_NEW_TAB_URL);
> 
> I do not understand this line at all now. What happened with my request for
> closeAllTabs()? Why are we opening a new tab here?

We don't want to call closeAllTabs here.
We want to avoid closing the last tab when calling page.close() since that will also close the window.

The only thing we need here is to open a new tab before issuing the close command on the page.
closeAllTabs() does not open a new tab, it just loads about:newtab in the last remaining tab
Attached patch v4.6.2.patch (obsolete) — Splinter Review
Attachment #8518722 - Flags: review?(andrei.eftimie)
Attached patch v4.6.2.patch (obsolete) — Splinter Review
Fixed the firefox/lib/tests/manifest.ini file.
Moved the openNewTab statement from close() method in BaseInContentPage class to two lines.
Attachment #8518234 - Attachment is obsolete: true
Attachment #8518722 - Attachment is obsolete: true
Attachment #8518722 - Flags: review?(andrei.eftimie)
Attachment #8518729 - Flags: review?(andrei.eftimie)
Attachment #8518729 - Flags: review?(andreea.matei)
(In reply to Andrei Eftimie from comment #41)
> We don't want to call closeAllTabs here.
> We want to avoid closing the last tab when calling page.close() since that
> will also close the window.

What? Please have a look at this method! It does not close the last tab, but loads about:newtab. That is exactly what we want here.
Comment on attachment 8518729 [details] [diff] [review]
v4.6.2.patch

Review of attachment 8518729 [details] [diff] [review]:
-----------------------------------------------------------------

We are in discussion and I don't see the point for having an updated patch with review requests.
Attachment #8518729 - Flags: review?(andrei.eftimie)
Attachment #8518729 - Flags: review?(andreea.matei)
(In reply to Henrik Skupin (:whimboo) from comment #45)
> (In reply to Andrei Eftimie from comment #41)
> > We don't want to call closeAllTabs here.
> > We want to avoid closing the last tab when calling page.close() since that
> > will also close the window.
> 
> What? Please have a look at this method! It does not close the last tab, but
> loads about:newtab. That is exactly what we want here.

Yep exactly. I was talking about what we want in this method.

We would have 2 options.

1) If only 1 tab available, use closeAllTabs() and return early (since the rest of the code will attempt to close the tab and we would be left with no open tabs. In this case we would not use the callback method even if specified

2) If only one tab available, open a new one so we can close the page tab with whatever method was specified.
Ah, I see. Option 2 is not that optimal, but it will allow us to go through the callback. That might indeed be the better method. So lets take that, but make sure to add a comment why we are doing that. It should be clear when inspecting the code.
Attached patch v4.6.2.patch (obsolete) — Splinter Review
Added explanatory comments to the *openTab()* statement in close() method of BaseInContentPage class.
Attachment #8518729 - Attachment is obsolete: true
Attachment #8518768 - Flags: review?(andrei.eftimie)
Attachment #8518768 - Flags: review?(andreea.matei)
Comment on attachment 8518768 [details] [diff] [review]
v4.6.2.patch

Review of attachment 8518768 [details] [diff] [review]:
-----------------------------------------------------------------

Ask a final r from Henrik with the updated comment.

All good from me otherwise.

::: firefox/lib/ui/base-in-content-page.js
@@ +140,5 @@
> +    }
> +
> +    // Handle the case when only one tab is opened
> +    if (this.browserWindow.tabs.length === 1) {
> +      // Open a new tab allowing to close the BICP tab with one of the method below

Please do merge this comment with the one above.
Attachment #8518768 - Flags: review?(andrei.eftimie)
Attachment #8518768 - Flags: review?(andreea.matei)
Attachment #8518768 - Flags: review+
Attached patch v4.6.2.patch (obsolete) — Splinter Review
> ::: firefox/lib/ui/base-in-content-page.js
> @@ +140,5 @@
> > +    }
> > +
> > +    // Handle the case when only one tab is opened
> > +    if (this.browserWindow.tabs.length === 1) {
> > +      // Open a new tab allowing to close the BICP tab with one of the method below
> 
> Please do merge this comment with the one above.

Done.
Attachment #8518768 - Attachment is obsolete: true
Attachment #8518912 - Flags: review?(hskupin)
Comment on attachment 8518912 [details] [diff] [review]
v4.6.2.patch

Review of attachment 8518912 [details] [diff] [review]:
-----------------------------------------------------------------

r=me library changes wise. Please update the nits and the test accordingly. No need for a review from me again. Thanks.

::: firefox/lib/tests/testBaseInContentPage.js
@@ +21,5 @@
> +}
> +
> +function testBaseInContentPage() {
> +  page.open(() => {
> +    addonsManager.open({type: "menu", waitFor: false});

I was thinking more of the shortcut `Accel+Shift+A` here. Using the addon manager directly will not be optimal, because it will become a subclass for BaseInContentPage, and needs testing there.

::: firefox/lib/ui/base-in-content-page.js
@@ +138,5 @@
> +    if (!this.contentWindow || this.contentWindow.closed) {
> +      assert.fail("Content window has been found");
> +    }
> +
> +    // Handle the case when only one tab is opened

nit: is visible

@@ +139,5 @@
> +      assert.fail("Content window has been found");
> +    }
> +
> +    // Handle the case when only one tab is opened
> +    // Open a new tab allowing to close the BICP tab with one of the method below

nit: Opening a new tab allows to close...
Attachment #8518912 - Flags: review?(hskupin) → review+
Attached patch v4.7.patch (obsolete) — Splinter Review
Using the keyboard shortcut to open the addons manager now in testBaseInContentPage.js.
Fixed nits.
Attachment #8518912 - Attachment is obsolete: true
Attachment #8520559 - Flags: review?(andrei.eftimie)
Attachment #8520559 - Flags: review?(andreea.matei)
Comment on attachment 8520559 [details] [diff] [review]
v4.7.patch

Review of attachment 8520559 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tests/testBaseInContentPage.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Include required modules
> +var addons = require("../../../lib/addons");

This is not used anymore. You can remove it.

@@ +23,5 @@
> +}
> +
> +function testBaseInContentPage() {
> +  page.open(() => {
> +    var cmdKey = utils.getEntity(dtds, "addons.commandkey");

Maybe we want to wait for the other patch to land so you can use browserWindow.getEntity()?
Depends on: 1084333
Attached patch v4.7.1.patchSplinter Review
The patch from Bug - 1084333 has landed on default.

Updated to browserWindow.getEntity() in testBaseInContentPage.
Attachment #8520559 - Attachment is obsolete: true
Attachment #8520559 - Flags: review?(andrei.eftimie)
Attachment #8520559 - Flags: review?(andreea.matei)
Attachment #8521450 - Flags: review?(andrei.eftimie)
Attachment #8521450 - Flags: review?(andreea.matei)
Comment on attachment 8521450 [details] [diff] [review]
v4.7.1.patch

Review of attachment 8521450 [details] [diff] [review]:
-----------------------------------------------------------------

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/018f583d3d1d (default)
Attachment #8521450 - Flags: review?(andrei.eftimie)
Attachment #8521450 - Flags: review?(andreea.matei)
Attachment #8521450 - Flags: review+
Attachment #8521450 - Flags: checkin+
All good on Beta as well. Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/728deb98f377 (mozilla-beta)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: