Closed
Bug 1079725
Opened 10 years ago
Closed 10 years ago
Add a BaseInContentPage class
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)
Mozilla QA Graveyard
Mozmill Tests
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)
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.
Comment 1•10 years ago
|
||
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.
Reporter | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•10 years ago
|
||
(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(); }
Comment 5•10 years ago
|
||
(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.
Reporter | ||
Comment 6•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lib][lang=js] → [lib][lang=js][Blocked by bug 1081014][Blocked by bug 1081851]
Updated•10 years ago
|
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 7•10 years ago
|
||
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-
Reporter | ||
Comment 8•10 years ago
|
||
(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 ?
Comment 9•10 years ago
|
||
(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.
Reporter | ||
Comment 10•10 years ago
|
||
Thanks, updated!
Attachment #8504588 -
Attachment is obsolete: true
Attachment #8506039 -
Flags: review?(andrei.eftimie)
Attachment #8506039 -
Flags: review?(andreea.matei)
Reporter | ||
Comment 11•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lib][lang=js][Blocked by bug 1081014][Blocked by bug 1081851][sprint] → [lib][lang=js][Blocked by bug 1081014][sprint]
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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-
Reporter | ||
Comment 14•10 years ago
|
||
(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)
Reporter | ||
Comment 15•10 years ago
|
||
> @@ +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.
Reporter | ||
Comment 16•10 years ago
|
||
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)
Reporter | ||
Comment 17•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
(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)
Comment 19•10 years ago
|
||
(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 20•10 years ago
|
||
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+
Reporter | ||
Comment 21•10 years ago
|
||
(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.
Comment 22•10 years ago
|
||
(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.
Reporter | ||
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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-
Reporter | ||
Comment 25•10 years ago
|
||
Attachment #8509403 -
Attachment is obsolete: true
Attachment #8512016 -
Flags: review?(andrei.eftimie)
Attachment #8512016 -
Flags: review?(andreea.matei)
Comment 26•10 years ago
|
||
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-
Reporter | ||
Comment 27•10 years ago
|
||
(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)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lib][lang=js][Blocked by bug 1081014][sprint] → [lib][lang=js][sprint]
Comment 28•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: danisielm → teodor.druta
Assignee | ||
Comment 29•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8513410 -
Attachment is obsolete: true
Comment 30•10 years ago
|
||
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-
Assignee | ||
Comment 31•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8515978 -
Flags: review?(hskupin)
Attachment #8515978 -
Flags: review?(andrei.eftimie)
Attachment #8515978 -
Flags: review?(andreea.matei)
Attachment #8515978 -
Flags: review+
Comment 32•10 years ago
|
||
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+
Assignee | ||
Comment 33•10 years ago
|
||
(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 34•10 years ago
|
||
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+
Assignee | ||
Comment 35•10 years ago
|
||
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 36•10 years ago
|
||
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 37•10 years ago
|
||
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)
Assignee | ||
Comment 38•10 years ago
|
||
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 39•10 years ago
|
||
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 40•10 years ago
|
||
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?
Comment 41•10 years ago
|
||
(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.
Comment 42•10 years ago
|
||
closeAllTabs() does not open a new tab, it just loads about:newtab in the last remaining tab
Assignee | ||
Comment 43•10 years ago
|
||
Attachment #8518722 -
Flags: review?(andrei.eftimie)
Assignee | ||
Comment 44•10 years ago
|
||
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)
Comment 45•10 years ago
|
||
(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 46•10 years ago
|
||
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)
Comment 47•10 years ago
|
||
(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.
Comment 48•10 years ago
|
||
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.
Assignee | ||
Comment 49•10 years ago
|
||
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 50•10 years ago
|
||
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+
Assignee | ||
Comment 51•10 years ago
|
||
> ::: 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 52•10 years ago
|
||
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+
Assignee | ||
Comment 53•10 years ago
|
||
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 54•10 years ago
|
||
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()?
Assignee | ||
Comment 55•10 years ago
|
||
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 56•10 years ago
|
||
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+
Comment 57•10 years ago
|
||
And lets get this in Aurora as well: https://hg.mozilla.org/qa/mozmill-tests/rev/d196f46116e1 (mozilla-aurora)
status-firefox33:
--- → unaffected
status-firefox34:
--- → affected
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
status-firefox-esr31:
--- → unaffected
Comment 58•10 years ago
|
||
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
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•