Closed
Bug 455553
Opened 16 years ago
Closed 12 years ago
New Tab Page feature
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
RESOLVED
FIXED
Firefox 12
People
(Reporter: asaf, Assigned: ttaubert)
References
(Depends on 4 open bugs)
Details
(Keywords: addon-compat, Whiteboard: [a11y-ping=davidb])
Attachments
(6 files, 44 obsolete files)
111.23 KB,
image/png
|
Details | |
24.53 KB,
patch
|
Details | Diff | Splinter Review | |
23.70 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
16.54 KB,
patch
|
Details | Diff | Splinter Review | |
38.73 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
61.61 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•16 years ago
|
||
* A new chrome page in place, opened when opening a new tab (but not when opening a new window). * The url is never shown in the location bar * The chrome page has access to the browser window, the previously opened tab and to the selection within that tab. * I tried to alter sessionstore service so that it doesn't store that page, still doesn't work.
Comment 2•16 years ago
|
||
(In reply to comment #1) > * I tried to alter sessionstore service so that it doesn't store that page Why would we restore a blank tab when the last thing the user saw was a non-blank one? AFAICT this new page isn't in any way special to how SessionStore should handle it - it will leave a trace in a tab's history, won't it (i.e. you'll be able to navigate back to the new tab page)?
Reporter | ||
Comment 3•16 years ago
|
||
You're not restoring blank tabs in sessionstore right now, are you? I'm going to make sure that you cannot navigate back to the new tab page, it's not supposed to be part of the session history.
Comment 4•16 years ago
|
||
Comment on attachment 338928 [details] [diff] [review] prototype v0.01 (In reply to comment #3) > You're not restoring blank tabs in sessionstore right now, are you? No. That's mostly due to the fact that blank tabs aren't in a tab's history, though, not because of treating about:blank in a special way ourselves. > I'm going to make sure that you cannot navigate back to the new tab page, Ideally, this could be achieved in our nsISHistory implementation (in which case your SessionStore modifications would work). Should you however choose to hack your way around, you'll want to remove the newTab.xul entry in nsSessionStore.js slightly above the place you've modified: >diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js > else if (history && history.count > 0) { > for (var j = 0; j < history.count; j++) > tabData.entries.push(this._serializeHistoryEntry(history.getEntryAtIndex(j, false), > aFullData)); > tabData.index = history.index + 1;
Updated•16 years ago
|
OS: Mac OS X → All
Hardware: PC → All
What kind of content is going to be shown on this page? I'm thinking something like the following would be nice: Speed Dial - https://addons.mozilla.org/en-US/firefox/addon/4810 Fast Dial - https://addons.mozilla.org/en-US/firefox/addon/5721 Should show recent things like starred and bookmarked pages and closed tabs along with most visited pages...all of them with previews, title and favicon. The reason why I strongly suggest starred pages is because they most likely often are used because they are so hard to find hidden away in the library. Those are probably the most useful thing to show on this page.
Version: unspecified → Trunk
Hit commit too fast, (In reply to comment #1) > * The url is never shown in the location bar Would the user be able to bookmark this page so if they wanted as a homepage they will be able to use it? No point in only showing the content when only opening a new tab.
I don't know if comments are welcomed, but I've been using Opera a lot and Chrome too recently, both try to show useful stuff in a new tab and on startup, but both fail in my opinion... Because what they show is completely useless. Automatically populated Speed Dial in Chrome is polluted with redirect pages like the Google login system (various GMail pages take up the top 6 visited sites). Also Forums' refresh-after-post pages tend to show up. Other stuff like search widgets are already part of the UI, moreover, a Speed Dial is useful even when being finished with a page and wanting to navigate away, and there's "Most Visited" in the bookmark bar for this currently (with the same flaws as Chrome's Speed Dial). An empty page with an input box on (above) it is not intimidating at all! Look at Google's success!
Comment 9•16 years ago
|
||
How about a stripped and local version of the normal start page: http://www.google.nl/firefox?client=firefox-a&rls=org.mozilla:en-US:official e..g. with 'about:start'. Possibly combined with some material from: http://en-us.www.mozilla.com/en-US/firefox/features/ So that users are pointed to (new) features. The page has be very lean and mean to prevent Ts and Tp impact.
Comment 10•16 years ago
|
||
(In reply to comment #9) It's fine to show a description of features the first time for a new user, but not every time a new tab is opened! And a stripped down version of the Google startpage is a search box, Firefox already has a search box!
Comment 11•16 years ago
|
||
(In reply to comment #7) > (In reply to comment #1) > > * The url is never shown in the location bar > > Would the user be able to bookmark this page so if they wanted as a homepage > they will be able to use it? No point in only showing the content when only > opening a new tab. use a about:tabs or about:start like Alfred Kayser calls it please About Speed Dial / Fast Dial or how you want to call it; Make in the preferences panel a new tab with the option show 4/9/16 choices and then the urls of the pages that you want to have. Now we have tags, and I have made a tag for pages I am using (almost) every day, in the list 'Most Visited' is the first page 'Google' and this is good to start Firefox with, but it isn't for browsing with (the search field of Firefox does it)
Updated•16 years ago
|
Target Milestone: --- → Future
Comment 12•16 years ago
|
||
How many of the ideas in Aza Raskin's blog (http://www.azarask.in/blog/post/firefox-31-new-tab-spec/) are likely to make it into this bug? Some of the ideas sound really excellent in principle. The contextual actions, like a map search of address details on the previously selected tab. The quick access strip that only contains sites that start history strands sounds like it may address to observation in comment 8 and avoid useless pages like gmail redirects. As long as the page loads virtually instantly then many of those ideas sound good. I am not sure how far off they are from being implemented - even in a try-server build.
Comment 13•16 years ago
|
||
This isn't going to make 3.1 if it doesn't get in for 3.1b2?
Comment 14•16 years ago
|
||
Note that the 'Ctrl-Tab' thing also has been removed from 3.1 (temporarily disabled), as it was seen as not yet completed / bug free. So, starting a new feature like the 'about:blank/start' thing is probably also something not for 3.1. Note, that the CtrlTab feature also has 3 rows of 3 previews, which also needs to be configurable, so this needs to be based on the same configs
Comment 15•15 years ago
|
||
(In reply to comment 14) > Note that the 'Ctrl-Tab' thing also has been removed from 3.1 (temporarily > disabled), as it was seen as not yet completed / bug free. I tried the add-on Showcase and it was a very configurable alternative to cover say All-Tabs features. We should also think about adding back an option to configure the new tab or "home" tab to always load just your "homepage" if you wanted - (useful for portals for organizations and end users who don't like blank new tabs and want to keep users focused to their site), as in providing a better option to re-implementing a wontfix bug 269664. -Just a note so we don't get lost: If this morphs to a one size fits all tab (which users don't expect, but) and no separate home button, there should be a new easy and consistent way for users to recognize and access their "homepage" from this new functionality to load their "homepage" ie: A home header, A home link, A home button, A home icon, A home image
Comment 17•13 years ago
|
||
This feature is coming back to life, and it's being tracked here: https://wiki.mozilla.org/New_Tab_page Here's a prototype I made that populates the new tab page with the same list of sites as the autocomplete dropdown. Since Test Pilot studies have indicated that a large percentage of users use that dropdown list, this seems like a reasonable place to start. Removing sites from this list has the same effect as removing them from the awesomebar popup (removing the page from browser history).
Assignee: mano → margaret.leibovic
Attachment #338928 -
Attachment is obsolete: true
Comment 18•13 years ago
|
||
Built from http://hg.mozilla.org/projects/ux/rev/dd1a9622f0dd Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110607 Firefox/7.0a1 Top sites on new tab invisible. Error: NewTabPage is undefined Source File: about:newtab Line: 1 Error: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://browser/content/newTab.js :: <TOP_LEVEL> :: line 43" data: no]
Comment 19•13 years ago
|
||
The import() in newTab.js is wrong - newTabUtils.jsm should probably be renamed to NewTabUtils.jsm, and the import() call should use "resource:///" instead of "resource://gre/" since this is a browser-only module (only matters for Firefox when omnijar is enabled, I think).
Comment 20•13 years ago
|
||
work now, just rename newTabUtils.jsm to NewTabUtils.jsm.
Updated•13 years ago
|
Summary: New Tab Feature → New Tab Page feature
Comment 21•13 years ago
|
||
If we're sticking with about:newtab as the url for this page, the following line also needs to be updated: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1549
Comment 22•13 years ago
|
||
It should be possible to add some feeds to the new tab page. http://upload.7pmtech.com/view/full/90_fltn2
Comment 23•13 years ago
|
||
Sorry, wrong link. Here's the correct one: http://upload.7pmtech.com/files/90_fltn2/2.jpg
Comment 24•13 years ago
|
||
You want to say in such a form as now, work is over?
Comment 25•13 years ago
|
||
(In reply to Tiger.711 from comment #24) > You want to say in such a form as now, work is over? I'm not actively working on this anymore. This feature is going to get more attention shortly, but I'm not going to be the one working on it.
Assignee: margaret.leibovic → nobody
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → ttaubert
Comment 26•13 years ago
|
||
I'm thinking the following would be nice: Desktop - https://addons.mozilla.org/en-US/firefox/addon/desktop/ thumbnails on new browser tab with full control on their layout(movable), sub-folders, and sync with Firefox Sync!
Comment 29•13 years ago
|
||
Previews are broken, they're all the same. It would be nice if a preview to the logos of sites (often it's logo.png) or possibility to use your own image as a preview. Will be able to change change the background image? Would it be possible to make it aero-glass, like in panorama? Will be able to add bookmarks? Like in Fast Dial and similar extensions.
Comment 30•13 years ago
|
||
May I put forward this proposal for the thumbnails on the new tab page? Currently there are at least three types of thumbnails across Firefox (Panorama, Fennec and those proposed for this). This takes some of the ideas that Faaborg/Margaret put forward and integrates them into the latest proposal for thumbnails on the new tab page.
Comment 31•13 years ago
|
||
As comment 30 hints at, is it an idea to combine new tab page and the 'panorama/tabview' into one thing?
Comment 32•13 years ago
|
||
(In reply to Alfred Kayser from comment #31) > As comment 30 hints at, is it an idea to combine new tab page and the > 'panorama/tabview' into one thing? Just to clarify, I was not hinting at combining the new tab page and panorama, but was rather hoping to have a consistent thumbnail style across the entire product line. My proposal merely build on the good work that Margaret done and integrated that into the current new tab page proposal while drawing some inspiration from panorama. As an aside, I would recommend the new tab page doesn't use a white background but rather appears as an extension to the navigation/bookmarks toolbar.
Assignee | ||
Comment 33•13 years ago
|
||
(In reply to Paul [sabret00the] from comment #30) > May I put forward this proposal for the thumbnails on the new tab page? > Currently there are at least three types of thumbnails across Firefox > (Panorama, Fennec and those proposed for this). I like your suggestion on first sight but there's some underlying work to be done first. I've created a thumbnail service in bug 497543 that needs to be used by all of these parts first (it's already used by the New Tab Page). After that is done the UX team would need to start figuring out a common design for all these "clients" (which is not too easy I think). I think this should definitely be handled in its own bug.
Comment 34•13 years ago
|
||
Hovered thumbnails should look more different from others. Perhaps the default opacity should be reduced.
Comment 35•13 years ago
|
||
Really nice implementation as of now expect the fact IMO the thumbnails can take lil more space and can be lil less congested ... Also , i think if a thumbnail is dragged outside the window area , it should be opened in new window , that's how i see it and will be more easy for use. I noticed that their size doesn't change with window size , can't we use CSS or something like that , as demoed everywhere , it will be good to promote these technologies IF POSSIBLE.
Comment 36•13 years ago
|
||
I noticed that there is a delay when the page loads up all the thumbnails from their respective websites. Caching this be somewhere will improve the perceived performance. For some sites like gmail.com, where the url always redirects to some other page, the new tab page keeps showing http://gmail.com as the thumbnail label instead of the page redirected to. Please fix this. Some UI suggestions: The page label should be displayed below the thumbnail like in panorama and not over it to allow better visibility. The URL should be hidden by default like in other browsers and may be displayed in a tooltip if needed. The black bars look ugly and should be replaced by just buttons as Paul posted above. Also these buttons may be displayed only on hover. The top left corner may show the page favicon and be replaced by the pin option on hover.
Comment 37•13 years ago
|
||
I agree with the thumbnail review proposal, only that I suggest, the pin should always be visible if a cell is pinned, so that the user knows that it is pinned just by looking at it.
Comment 38•13 years ago
|
||
The grid should be 2X4 instead of 3X3 to make better use of screen space, and allow bigger thumbnails. This will also allow placing the site title below the thumbnail and not over it.
Comment 39•13 years ago
|
||
I think, that the grid should be customizable.
Comment 40•13 years ago
|
||
Is it reasonable to expect the layout to adapt to screensize based on CSS media queries?
Comment 41•13 years ago
|
||
Tim, on OS X, dropping a thumbnail is janky, because the OS does a slow animation for drags between the mouse being released and when dragend is called, which is perceived as an unnecessary delay in our use cases. I ran into this problem when implementing tab animations too. To circumvent this, I recommend calling event.preventDefault() in a dragover handler on the document, so that the OS thinks that the drop is accepted and skips the animation.
Assignee | ||
Comment 42•13 years ago
|
||
(In reply to Frank Yan (:fryn) from comment #41) > Tim, on OS X, dropping a thumbnail is janky, because the OS does a slow > animation for drags between the mouse being released and when dragend is > called, which is perceived as an unnecessary delay in our use cases. I ran > into this problem when implementing tab animations too. To circumvent this, > I recommend calling event.preventDefault() in a dragover handler on the > document, so that the OS thinks that the drop is accepted and skips the > animation. Ok, fixed in my patch queue :) Thanks!
Assignee | ||
Comment 43•13 years ago
|
||
Asking for UX review. This is especially about basics, effects and drag/drop behavior. The following improvement bugs exist which we want to tackle on their own to get this basic feature reviewed. We could (and probably would) of course defer landing the whole feature until any or all of these bugs have been fixed: bug 705000, bug 705001 Please use tomorrow's UX Nightly (should be 2011-11-25).
Attachment #535455 -
Attachment is obsolete: true
Attachment #576861 -
Flags: ui-review?(jboriss)
Comment 44•13 years ago
|
||
Using the latest hourly's. Are the two buttons on top right intentionally left transparent ? I am talking about the buttons to reset the New Tab Page and to toggle the display of the 9 tabs. Also, while dragging any thumbnail, the mouse pointer flickers and changes back and forth from being a dragging mouse pointer to a normal mouse pointer.
Assignee | ||
Comment 47•13 years ago
|
||
(In reply to scrapmachines from comment #44) > Are the two buttons on top right intentionally left transparent ? > Also, while dragging any thumbnail, the mouse pointer flickers and changes > back and forth from being a dragging mouse pointer to a normal mouse pointer. FTR, that's on Windows. Thanks for reporting, should be fixed with today's nightly.
Assignee | ||
Comment 48•13 years ago
|
||
Attachment #576861 -
Attachment is obsolete: true
Attachment #576861 -
Flags: ui-review?(jboriss)
Assignee | ||
Comment 49•13 years ago
|
||
Assignee | ||
Comment 50•13 years ago
|
||
Assignee | ||
Comment 51•13 years ago
|
||
Assignee | ||
Comment 52•13 years ago
|
||
Comment 53•13 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #47) > (In reply to scrapmachines from comment #44) > > Are the two buttons on top right intentionally left transparent ? > > Also, while dragging any thumbnail, the mouse pointer flickers and changes > > back and forth from being a dragging mouse pointer to a normal mouse pointer. > > FTR, that's on Windows. Thanks for reporting, should be fixed with today's > nightly. Totally works on Windwos now , both the icons on the top right and the flickering. Great work. It should be ready now to land to mc.
Assignee | ||
Updated•13 years ago
|
Attachment #579643 -
Flags: feedback?(jwein)
Assignee | ||
Updated•13 years ago
|
Attachment #579644 -
Flags: feedback?(bmcbride)
Assignee | ||
Updated•13 years ago
|
Attachment #579645 -
Flags: feedback?(fryn)
Assignee | ||
Updated•13 years ago
|
Attachment #579646 -
Flags: feedback?(mak77)
Assignee | ||
Updated•13 years ago
|
Attachment #579647 -
Flags: feedback?(dietrich)
Comment 54•13 years ago
|
||
Comment on attachment 579647 [details] [diff] [review] Part 5 - New Tab Page tests and test suite Review of attachment 579647 [details] [diff] [review]: ----------------------------------------------------------------- need to document all the functions in head.js, including an overview of how the tests in this dir work (aSitePattern, etc).
Attachment #579647 -
Flags: feedback?(dietrich) → feedback+
Comment 55•13 years ago
|
||
Comment on attachment 579643 [details] [diff] [review] Part 1 - XHTML Page / Scripts Review of attachment 579643 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! There are many places where double underscores are used to prefix named functions that can be switched to use a single underscore, and a few places where |__defineGetter__| is used that can be switched to use |Object.defineProperty| instead. Also, there are a number of |addEventListener| calls that don't appear to have matching |removeEventListener| calls. These may be innocuous, but it is worth double-checking. ::: browser/base/content/newtab/cells.js @@ +39,5 @@ > +// Class: Cell > +// This class represents a cell contained in the grid. > +function Cell(aNode) { > + this._node = aNode; > + this._node.__newtabCell = this; We should probably use a single underscore prefix here, such as |this._node._newtabCell| @@ +58,5 @@ > + get index() { > + let index = Grid.cells.indexOf(this); > + > + // cache this value, overwrite the getter > + this.__defineGetter__("index", function () index); Can you use Object.defineProperty() here? According to this blog post, Object.defineProperty is the preferred approach now: http://whereswalden.com/2010/04/16/more-spidermonkey-changes-ancient-esoteric-very-rarely-used-syntax-for-creating-getters-and-setters-is-being-removed/ @@ +69,5 @@ > + let prev = this.node.previousElementSibling; > + prev = prev && prev.__newtabCell; > + > + // cache this value, overwrite the getter > + this.__defineGetter__("previousSibling", function () prev); Same Object.defineProperty() question here. @@ +80,5 @@ > + let next = this.node.nextElementSibling; > + next = next && next.__newtabCell; > + > + // cache this value, overwrite the getter > + this.__defineGetter__("nextSibling", function () next); Ditto. @@ +88,5 @@ > + > + // the site contained in the cell, if any > + get site() { > + let firstChild = this.node.firstElementChild; > + return firstChild && firstChild.__newtabSite; Same nit about the double underscores here. ::: browser/base/content/newtab/drag.js @@ +141,5 @@ > + // > + // Parameters: > + // aSite - the site that's being dragged > + // aEvent - the 'dragstart' event > + _setDragData: function Drag__setDragData(aSite, aEvent) { Can you change to a single underscore here on the named function? @@ +154,5 @@ > + dt.setData("text/html", "<a href=\"" + url + "\">" + url + "</a>"); > + > + // create and use an empty drag image. we don't want to use the default > + // drag image with its default opacity. > + let img = document.createElement("div"); Is this supposed to be a div or an img? Can you change the variable name to match the tag name? ::: browser/base/content/newtab/drop.js @@ +36,5 @@ > + * ***** END LICENSE BLOCK ***** */ > + > +// a little delay that prevents the grid from being too sensitive when dragging > +// sites around > +const DELAY_REARRANGE = 250; Can you please rename this to DELAY_REARRANGE_MS? I prefer having constant numbers with units. @@ +101,5 @@ > + // Re-pins all pinned sites in their (new) positions. > + // > + // Parameters: > + // aCell - the drop target cell > + _repinSitesAfterDrop: function Drop__repinSitesAfterDrop(aCell) { Single underscore here? @@ +110,5 @@ > + return aSite && aSite.isPinned(); > + }); > + > + // re-pin all shifted pinned cells > + pinnedSites.forEach(function (aSite) aSite.pin(sites.indexOf(aSite))); This file is included from a "use strict"; file. So |this| should be set to the passed argument, which I believe is |undefined|. Can you pass a |thisArg| to |forEach| so future authors will know what to expect. @@ +120,5 @@ > + // > + // Parameters: > + // aCell - the drop target cell > + // aEvent - the 'dragexit' event > + _pinDraggedSite: function Drop__pinDraggedSite(aCell, aEvent) { Single underscore here? @@ +142,5 @@ > + // Time a rearrange with a little delay. > + // > + // Parameters: > + // aCell - the drop target cell > + _delayedRearrange: function Drop__delayedRearrange(aCell) { Ditto. @@ +164,5 @@ > + > + // ---------- > + // Function: _cancelDelayedArrange > + // Cancels a timed rearrange, if any. > + _cancelDelayedArrange: function Drop__cancelDelayedArrange() { Ditto. @@ +177,5 @@ > + // Rearrange all sites in the grid depending on the current drop target. > + // > + // Parameters: > + // aCell - the drop target cell > + _rearrange: function Drop__rearrange(aCell) { Ditto. ::: browser/base/content/newtab/dropPreview.js @@ +169,5 @@ > + return range; > + }, > + > + // ---------- > + // Function: _filterPinnedSites This looks like an incorrect copy/paste. @@ +176,5 @@ > + // > + // Parameters: > + // aSites - the array of sites to check > + // aCell - the drop target cell > + _hasOverflownPinnedSite: Hmm... overflown vs. overflowed... Can we change this to |_hasOverflowedPinnedSite| and change the other mentions to use overflowed instead? Overflown reminds me too much of airplanes missing runways :( @@ +199,5 @@ > + > + // ---------- > + // Function: _repositionOverflownPinnedSite > + // We have a overflown pinned site that we need to re-position so that it's > + // visible again. We try to find a lower-prio cell (empty or containing s/lower-prio/lower-priority ::: browser/base/content/newtab/dropTargetShim.js @@ +69,5 @@ > + Grid.lock(); > + }, > + > + // ---------- > + // Function: _start Copy/paste mistake? ::: browser/base/content/newtab/grid.js @@ +88,5 @@ > + // Function: refresh > + // Refreshes the grid and re-creates all sites. > + refresh: function Grid_refresh() { > + // remove all sites > + this.cells.forEach(function (cell) { Please add a |this| argument to the forEach call. @@ +140,5 @@ > + // ---------- > + // Function: _draw > + // Draws the grid, creates all sites and puts them into their cells. > + _draw: function Grid__draw() { > + let self = this; Do we need to add the |self| variable here? Shouldn't |this| be consistent within the function? Maybe this was left over from a previous version that used a closure? ::: browser/base/content/newtab/helper.js @@ +59,5 @@ > + debug("trace: " + aMsg + "\n" + stack.join("\n")); > +} > + > +// ########## > +// Class: Batch This is a cool little class. It took me a minute to understand the goal of the class, but now that I get it, I think it is pretty neat :) @@ +108,5 @@ > + // Checks if the batch has finished. > + _check: function Batch__check() { > + if (this._count == 0 && this._callback) > + this._callback(); > + } There seems to be an odd invariant here. Suppose a user of the Batch class does the following (pseudocode follows): let b = new Batch(function() console.log('callback')); b.close(); b.pop(); delete b; In this scenario, it looks like the callback will be invoked twice. While no sane author would intend to write the code this way, it should be trivial to restrict the callback from being invoked twice. ::: browser/base/content/newtab/newTab.css @@ +31,5 @@ > +.toolbar-button { > + position: absolute; > + cursor: pointer; > + background-color: transparent; > + background-image: url(chrome://browser/skin/newtab/toolbar.png); This should probably be moved to a theme specific CSS file (in /browser/themes/). @@ +83,5 @@ > +#toolbar-button-reset:active { > + background-position: -21px -24px; > +} > + > +.modified #toolbar-button-reset, .disabled #toolbar-button-show { We should probably stay away from using the descendent selector. Can you apply these classes to the descendent elements? In other words, adjust the code so these selectors can be |#toolbar-button-reset.modified, #toolbar-button-show.disabled|. @@ +88,5 @@ > + opacity: 1; > + pointer-events: auto; > +} > + > +.disabled #toolbar-button-hide, .disabled #toolbar-button-reset { Ditto. @@ +143,5 @@ > + width: 201px; > + height: 127px; > + outline: none; > + background-color: #ececec; > + box-shadow: 0 0 0 #000; The box-shadow can't be seen here because it has no offset and no blur radius. This is probably here for the transition, but I did some simple tests and I think you can remove this line. I tested with these URIs: data:text/html,<style>div{-moz-transition:200ms;box-shadow:0 0 0 black;} div:hover{box-shadow:0 100px 2px black}</style><div style="background-color:red; width:400px; height:200px;">box-shadow</div> data:text/html,<style>div{-moz-transition:200ms;box-shadow:none;} div:hover{box-shadow:0 100px 2px black}</style><div style="background-color:red; width:400px; height:200px;">box-shadow</div> data:text/html,<style>div{-moz-transition:200ms;} div:hover{box-shadow:0 100px 2px black}</style><div style="background-color:red; width:400px; height:200px;">box-shadow</div> @@ +193,5 @@ > + font-weight: 700; > + font-size: 70%; > + text-decoration: none; > + line-height: 1.2em; > + background: rgba(0, 0, 0, 0.8); Please change this to |background-color: rgba(0,0,0,0.8);|. @@ +284,5 @@ > +/* DRAG & DROP */ > +.drag-img { > + width: 1px; > + height: 1px; > + background: none #fff; Can you use |background-color: #fff;| here instead? @@ +285,5 @@ > +.drag-img { > + width: 1px; > + height: 1px; > + background: none #fff; > + opacity: 0.01; I'm not sure why the opacity is 0.01. Is this for a transition? ::: browser/base/content/newtab/newTab.xhtml @@ +49,5 @@ > +]> > + > +<html xmlns="http://www.w3.org/1999/xhtml"> > + <head> > + <title>New Tab</title> This text should probably be moved out to a DTD file. ::: browser/base/content/newtab/toolbar.js @@ +74,5 @@ > + Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, false); > + }, > + > + // ---------- > + // Function: hide Copy/paste mistake? @@ +80,5 @@ > + // > + // Parameters: > + // aCallback - the callback to call when the page has been reset > + reset: function Toolbar_reset(aCallback) { > + if (aCallback && typeof aCallback != "function") There is similar code throughout this patch that takes a callback argument, checks for truthiness, and invokes it if possible. This is the only place within the patch that checks the |typeof| the callback. Does |Toolbar_reset| sometimes get called with a non-function |aCallback| argument? If not, should this check be removed or should the other callback invocations check the |typeof| the callback?
Attachment #579643 -
Flags: feedback?(jwein) → feedback+
Comment 56•13 years ago
|
||
Comment on attachment 579645 [details] [diff] [review] Part 3 - about:newtab integration This looks good, but I highly recommend we add code either to hide the favicon in the tab for the New Tab Page or to suppress the throbber, since the throbber makes opening new tabs feel slower. Also, the New Tab Page is very high contrast right now. That, combined with the brief blank page shown to the user before the page is rendered, is quite distracting. Could we mitigate that by toning down the contrast (via colors or opacity)?
Attachment #579645 -
Flags: feedback?(fryn) → feedback+
Comment 57•12 years ago
|
||
Comment on attachment 579644 [details] [diff] [review] Part 2 - Assets / Images Review of attachment 579644 [details] [diff] [review]: ----------------------------------------------------------------- These jar.mn files look SPLENDID!
Attachment #579644 -
Flags: feedback?(bmcbride) → feedback+
Comment 58•12 years ago
|
||
Comment on attachment 579643 [details] [diff] [review] Part 1 - XHTML Page / Scripts Review of attachment 579643 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/animations.js @@ +32,5 @@ > + * and other provisions required by the GPL or the LGPL. If you do not delete > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ Since these files are being #include'd, and the base file has the same license, it'd be nice to have the licenses here preprocessed away so you don't end up with a file with a dozen licenses - which seems like a waste of space. ::: browser/base/content/newtab/cells.js @@ +41,5 @@ > +function Cell(aNode) { > + this._node = aNode; > + this._node.__newtabCell = this; > + > + // register drag-and-drop event handlers General nit: Capital first letter, full stop at the end. @@ +45,5 @@ > + // register drag-and-drop event handlers > + ["dragenter", "dragover", "dragexit", "drop"].forEach(function (aType) { > + let method = "_" + aType; > + this[method] = this[method].bind(this); > + this._node.addEventListener(aType, this[method], false); Could make this more deliberate and potentially less error prone (maintenance-wise) by having the prefix be "_on" or "on", and capitalizing the event names (addEventListener is case-insensitive), so you end up pointing to functions like _onDragOver. ::: browser/base/content/newtab/grid.js @@ +133,5 @@ > + ' <input class="button strip-button strip-button-block" type="button"/>' + > + '</span>'; > + > + let fragment = this._siteFragment = document.createDocumentFragment(); > + fragment.appendChild(site); This feels awkward - no need for the temporary variable |fragment|. ::: browser/base/content/newtab/helper.js @@ +40,5 @@ > +// Logs a given debug message to the console. > +// > +// Parameters: > +// aMsg - the log message > +function debug(aMsg) { This appears to be unused. @@ +51,5 @@ > +// Logs a given debug message to the console and prints the current stack trace. > +// > +// Parameters: > +// aMsg - the log message > +function trace(aMsg) { This appears to be unused. ::: browser/base/content/newtab/newTab.css @@ +128,5 @@ > + margin: 0 0 15px 16px; > +} > + > +.cell:nth-child(3n+3) { > + margin-right: 0; This doesn't seem RTL friendly. ::: browser/base/content/newtab/newTab.xhtml @@ +47,5 @@ > + <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd" > > + %browserDTD; > +]> > + > +<html xmlns="http://www.w3.org/1999/xhtml"> HTML5 is the new hotness. ::: browser/base/content/newtab/page.js @@ +38,5 @@ > +// ########## > +// Class: Page > +// This singleton represents the whole 'New Tab Page' and takes care of > +// initializing all its components. > +let Page = { Since this is used as a global variable (Grid, Animations, etc too), it's name should reflect it. "Page" makes it look like you should use |new Page()|, which is misleading. gPage or similar would be better. Alternatively, you could do |let gPage = new Page("#toolbar", "#grid")|. That would also let you put this (and a bunch of other files here) in a JSM - would be interesting to see if doing that could reduce the load time by a few milliseconds (which can really matter here), since the JS wouldn't need to be parsed every time. ::: browser/base/content/newtab/updater.js @@ +38,5 @@ > +// ########## > +// Class: Updater > +// This singleton provides functionality to update the current grid to a new > +// set of pinned and blocked sites. It adds, moves and removes sites. > +let Updater = { I wonder if this should be merged with Grid.
Attachment #579643 -
Flags: feedback+
Comment 59•12 years ago
|
||
Comment on attachment 579646 [details] [diff] [review] Part 4 - Shared Module Review of attachment 579646 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/NewTabUtils.jsm @@ +60,5 @@ > + "@mozilla.org/dom/storagemanager;1", "nsIDOMStorageManager"); > + > +// ########## > +// Class: Storage > +// Singleton that provides storage functionality. My kingdom for consistent code documentation style. And by that I mean javadoc-style, which is the predominant style for JS files in the tree (I have no data to back that up). @@ +71,5 @@ > + return this._storage; > + > + let uri = Services.io.newURI("about:newtab", null, null); > + let principal = gScriptSecurityManager.getCodebasePrincipal(uri); > + let storage = gStorageManager.getLocalStorageForPrincipal(principal, ""); Looks like this won't use PrivateBrowsingStorage if the first about:newtab is opened when the browser is already in private-browsing mode. eg, if Firefox is started with the -privatebrowsing command line switch (or whatever it's called). @@ +132,5 @@ > + return { > + observe: function () { > + if (gPrivateBrowsing.privateBrowsingEnabled) { > + // create a temporary storage for the pb mode > + self._storage = new PrivateBrowsingStorage(self.storage); Add a comment here saying why resetCache()/update() is only called when leaving private browsing mode, as it isn't immediately obvious. @@ +293,5 @@ > + // > + // Parameters: > + // aLink - the link to check > + isPinned: function PinnedLinks_isPinned(aLink) { > + return (-1 < this._indexOfLink(aLink)); |this._indexOfLink(aLink) != -1| makes it more obvious what the code is doing, IMO. @@ +474,5 @@ > +// ########## > +// Class: Testing > +// Singleton that provides testing functionality and wrapped access to core > +// objects to mock certain values. > +let Testing = { Is there any way around needing this? I cringe at shipping code that's only useful for automated tests. @@ +532,5 @@ > + Pages: Pages, > + Links: Links, > + Testing: Testing, > + PinnedLinks: PinnedLinks, > + BlockedLinks: BlockedLinks This would look less like constructors if they were (for instance) NewTabUtils.pages instead of NewTabUtils.Pages. Also, pages/page is subtle - allPages/page is less subtle. ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +457,4 @@ > var searchParamParts = aSearchParam.split(" "); > this._enableActions = searchParamParts.indexOf("enable-actions") != -1; > > + if (searchParamParts.indexOf("newtab-maxresults") != -1) This feels a little wrong/hackish. Calling it something like "guarentee-more-results" or something might be better. I bet Macro has thoughts (hopefully better than mine). @@ +457,5 @@ > var searchParamParts = aSearchParam.split(" "); > this._enableActions = searchParamParts.indexOf("enable-actions") != -1; > > + if (searchParamParts.indexOf("newtab-maxresults") != -1) > + this._maxRichResults = 100; And this should probably be a global constant.
Attachment #579646 -
Flags: feedback+
Assignee | ||
Comment 60•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #54) > need to document all the functions in head.js, including an overview of how > the tests in this dir work (aSitePattern, etc). Done.
Attachment #579647 -
Attachment is obsolete: true
Attachment #581427 -
Flags: review?(dietrich)
Assignee | ||
Comment 61•12 years ago
|
||
We need a way to get more than the default 25 results from the nsPlacesAutoComplete service.
Attachment #581428 -
Flags: review?(mak77)
Assignee | ||
Comment 62•12 years ago
|
||
(In reply to Frank Yan (:fryn) from comment #56) > This looks good, but I highly recommend we add code either to hide the > favicon in the tab for the New Tab Page or to suppress the throbber, since > the throbber makes opening new tabs feel slower. Done. > Also, the New Tab Page is very high contrast right now. That, combined with > the brief blank page shown to the user before the page is rendered, is quite > distracting. Could we mitigate that by toning down the contrast (via colors > or opacity)? I think that's gonna be handled in bug 705000.
Attachment #579645 -
Attachment is obsolete: true
Attachment #581432 -
Flags: review?(fryn)
Assignee | ||
Comment 63•12 years ago
|
||
(In reply to Blair McBride (:Unfocused) from comment #59) > My kingdom for consistent code documentation style. And by that I mean > javadoc-style, which is the predominant style for JS files in the tree (I > have no data to back that up). Documentation style changed to javadoc. > Looks like this won't use PrivateBrowsingStorage if the first about:newtab > is opened when the browser is already in private-browsing mode. eg, if > Firefox is started with the -privatebrowsing command line switch (or > whatever it's called). True, I didn't consider this. Fixed. > Add a comment here saying why resetCache()/update() is only called when > leaving private browsing mode, as it isn't immediately obvious. Done. > Is there any way around needing this? I cringe at shipping code that's only > useful for automated tests. Turns out that was legacy anyway so we don't need it anymore. Removed. > This would look less like constructors if they were (for instance) > NewTabUtils.pages instead of NewTabUtils.Pages. Also, pages/page is subtle - > allPages/page is less subtle. Fixed.
Attachment #579646 -
Attachment is obsolete: true
Attachment #581436 -
Flags: review?(bmcbride)
Attachment #579646 -
Flags: feedback?(mak77)
Assignee | ||
Comment 64•12 years ago
|
||
(In reply to Jared Wein [:jwein and :jaws] from comment #55) > ::: browser/base/content/newtab/newTab.css > > + background-image: url(chrome://browser/skin/newtab/toolbar.png); > > This should probably be moved to a theme specific CSS file (in /browser/themes/). Done. > We should probably stay away from using the descendent selector. Can you apply > these classes to the descendent elements? In other words, adjust the code so > these selectors can be |#toolbar-button-reset.modified, #toolbar-button-show.disabled|. Fixed using attributes. > The box-shadow can't be seen here because it has no offset and no blur radius. > This is probably here for the transition, but I did some simple tests and I think > you can remove this line. > @@ +285,5 @@ > > +.drag-img { > > + opacity: 0.01; > > I'm not sure why the opacity is 0.01. Is this for a transition? I want this to be hidden so that the DnD implementation does not use its default drag image. 'visibility: hidden' and 'opacity: 0' don't work - that seems to be equal to hidden. So this only works with 'opacity: 0.01'. Fixed all other minor CSS issues. (In reply to Blair McBride (:Unfocused) from comment #58) > ::: browser/base/content/newtab/newTab.css > @@ +128,5 @@ > > +.cell:nth-child(3n+3) { > > + margin-right: 0; > > This doesn't seem RTL friendly. Fixed using -moz-margin-end.
Attachment #579644 -
Attachment is obsolete: true
Attachment #581441 -
Flags: review?(jwein)
Comment 65•12 years ago
|
||
Comment on attachment 581441 [details] [diff] [review] Part 2 - Assets / CSS / Images Review of attachment 581441 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, although I should defer to someone who has review privileges on this.
Attachment #581441 -
Flags: review?(jwein)
Attachment #581441 -
Flags: review?(dao)
Attachment #581441 -
Flags: feedback+
Comment 66•12 years ago
|
||
Comment on attachment 581432 [details] [diff] [review] Part 3 - about:newtab integration Review of attachment 581432 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Tim Taubert [:ttaubert] from comment #62) > (In reply to Frank Yan (:fryn) from comment #56) > > Also, the New Tab Page is very high contrast right now. That, combined with > > the brief blank page shown to the user before the page is rendered, is quite > > distracting. Could we mitigate that by toning down the contrast (via colors > > or opacity)? > > I think that's gonna be handled in bug 705000. Okay, but I really, really want the thumbnail contrast problem addressed ASAP. By thumbnail contract, I also mean the thumbnail itself, not just its label. I don't want us to be both late to the new tab page party and worse in UX. ::: browser/app/profile/firefox.js @@ +1084,5 @@ > pref("browser.panorama.animate_zoom", true); > > +// New Tab feature > +pref("browser.newtab.enabled", true); > + Nit: I recommend changing the comment to "Enable New Tab Page (about:newtab)" or something like that, since new tab creation in general has been around for a while. ::: browser/base/content/tabbrowser.xml @@ +536,4 @@ > if (aWebProgress.DOMWindow == this.mBrowser.contentWindow) > this.mBrowser.userTypedClear += 2; > > + if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") { Interesting way of suppressing the throbber. :P
Attachment #581432 -
Flags: review?(fryn) → review+
Comment 67•12 years ago
|
||
Comment on attachment 581432 [details] [diff] [review] Part 3 - about:newtab integration >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >- if (title && title != "about:blank") { >+ if (title && title != "about:blank" && title != "about:newtab") { It might be handy to have symbolic functions that: - checks whether a given URL should be considered "blank" (for the moment that would just check for about:blank and about:newtab) - returns the URL to use for new tab pages (to be used in the ~3 places in this patch that currently hardcode about:newtab) > var blank = !aURI || (aURI == "about:blank"); > >- if (blank) >+ if (blank || aURI == "about:newtab") Can't you just change add this condition to the setting of "blank"? That way mBlank will be set correctly too.
Assignee | ||
Comment 68•12 years ago
|
||
(In reply to Jared Wein [:jwein and :jaws] from comment #55) > > + <title>New Tab</title> > This text should probably be moved out to a DTD file. Yeah, of course. Done. > There is similar code throughout this patch that takes a callback argument, > checks for truthiness, and invokes it if possible. This is the only place > within the patch that checks the |typeof| the callback. Does |Toolbar_reset| > sometimes get called with a non-function |aCallback| argument? If not, > should this check be removed or should the other callback invocations check > the |typeof| the callback? Actually this check was here because I directly used this function as an event handler. So when called directly we could pass a callback but this could have also been an event. I changed this so that these functions never get events (as they don't need them) and we can just check for |if (aCallback)|. > We should probably use a single underscore prefix here, such as > |this._node._newtabCell| I'm attaching an object to a DOM node so I figured it might be better to use a double underscore prefix as that's consistent with the code base I know that does the same (SessionStore, Panorama). > Can you use Object.defineProperty() here? Good point, using defineProperty() now. > > + _setDragData: function Drag__setDragData(aSite, aEvent) { > Can you change to a single underscore here on the named function? Done. > > + let img = document.createElement("div"); > Is this supposed to be a div or an img? Can you change the variable name to match > the tag name? Changed. > > +const DELAY_REARRANGE = 250; > Can you please rename this to DELAY_REARRANGE_MS? I prefer having constant numbers > with units. Done. > > + pinnedSites.forEach(function (aSite) aSite.pin(sites.indexOf(aSite))); > This file is included from a "use strict"; file. So |this| should be set to the > passed argument, which I believe is |undefined|. Can you pass a |thisArg| to > |forEach| so future authors will know what to expect. Done. > > + _hasOverflownPinnedSite: > Hmm... overflown vs. overflowed... Can we change this to > |_hasOverflowedPinnedSite| and change the other mentions to use overflowed > instead? Overflown reminds me too much of airplanes missing runways :( Changed. > In this scenario, it looks like the callback will be invoked twice. While no sane > author would intend to write the code this way, it should be trivial to restrict > the callback from being invoked twice. Fixed the Batch class. (In reply to Blair McBride (:Unfocused) from comment #58) > Since these files are being #include'd, and the base file has the same > license, it'd be nice to have the licenses here preprocessed away so you > don't end up with a file with a dozen licenses - which seems like a waste of > space. Done. > General nit: Capital first letter, full stop at the end. Fixed. > Could make this more deliberate and potentially less error prone > (maintenance-wise) by having the prefix be "_on" or "on", and capitalizing > the event names (addEventListener is case-insensitive), so you end up > pointing to functions like _onDragOver. Yep, that looks better, fixed. > Since this is used as a global variable (Grid, Animations, etc too), it's > name should reflect it. "Page" makes it look like you should use |new > Page()|, which is misleading. gPage or similar would be better. Renamed all singletons. > > +let Updater = { > I wonder if this should be merged with Grid. I actually like it more separated so that the whole Grid isn't messed up with update code. It's a bit cleaner IMHO.
Attachment #579643 -
Attachment is obsolete: true
Attachment #581451 -
Flags: review?(bmcbride)
Comment 69•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #67) > > var blank = !aURI || (aURI == "about:blank"); > > > >- if (blank) > >+ if (blank || aURI == "about:newtab") > > Can't you just change add this condition to the setting of "blank"? That way > mBlank will be set correctly too. The problem is that `blank` is also used for other things that should not apply to about:newtab (e.g. when `blank` is true, we don't bother to stop the about:blank load in anticipation of another document being loaded immediately afterward). I'm not sure what `mBlank` is used for though.
Assignee | ||
Comment 70•12 years ago
|
||
Yes, I tried setting 'blank' first but that didn't work out well.
Comment 71•12 years ago
|
||
Comment on attachment 581428 [details] [diff] [review] Part 6 - Extend nsPlacesAutoComplete to get more results discussed on IRC, I think the PlacesAutocomplete overhead is excessive for this task, while it could follow the same code path as Win 7 jumplists and still get a frecency ordered list. Clearing the request for now.
Attachment #581428 -
Flags: review?(mak77)
Assignee | ||
Comment 72•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #67) > Can't you just change add this condition to the setting of "blank"? That way > mBlank will be set correctly too. The progress listener sets |mBlank = false| after the first STATE_STOP so that doesn't work either with about:newtab and its images.
Comment 73•12 years ago
|
||
Comment on attachment 581451 [details] [diff] [review] Part 1 - XHTML Page / Scripts Review of attachment 581451 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/animations.js @@ +42,5 @@ > + */ > +let gAnimations = { > + /** > + * Fades in the given node from 0 to 1 opacity. > + * @param aNode the node to animate Global nit: Comments describing params/return values should be full sentences too. ie: * @param aNode The node to animate. @@ +79,5 @@ > + _start: function CssAnimations_start() { > + let node = this._node; > + let name = this._name; > + let classes = node.classList; > + let callback = this._callback; You set this._node, this._name, and this._callback in the constructor, but then just pull them out again here (which is only called by the constructor). Should either directly use this._node etc (preferred, IMO), or just pass them as parameters (or merge _start into the constructor, for that matter). ::: browser/base/content/newtab/drop.js @@ +102,5 @@ > + let sites = gDropPreview.rearrange(aCell); > + > + // Filter out pinned sites. > + let pinnedSites = sites.filter(function (aSite) { > + return aSite && aSite.isPinned(); Can aSite ever be null? Seems like it should be guaranteed by rearrange() to always be non-null. ::: browser/base/content/newtab/dropPreview.js @@ +93,5 @@ > + * @param aSites the array of sites containing the dragged site > + * @param aCell the drop target cell > + */ > + _repositionPinnedSites: > + function DropPreview_repositionPinnedSites(aSites, aCell) { Splitting it like this makes it look like a typo or an inner function. Reads easier if you split this line like so (even if it means breaking 80 characters, IMO): _repositionPinnedSites: function DropPreview_repositionPinnedSites(aSites, aCell) { With aCell lined up with aSites. Couple of other places like this too. @@ +95,5 @@ > + */ > + _repositionPinnedSites: > + function DropPreview_repositionPinnedSites(aSites, aCell) { > + > + // collect all pinned sites. Capital first letter. @@ +164,5 @@ > + return range; > + }, > + > + /** > + * Checks if the given array of sites contains a pinned sites that has "a pinned sites"? @@ +200,5 @@ > + */ > + _repositionOverflowedPinnedSite: > + function DropPreview_repositionOverflowedPinnedSite(aSites, aCell) { > + > + // Try to find a lower-prio cell (empty or containing an unpinned site). s/lower-prio/lower-priority/ ? @@ +238,5 @@ > + // Search (beginning with the last site in the grid) for a site that is > + // empty or unpinned (an thus lower-priority) and can be pushed out of the > + // grid instead of the pinned site. > + for (let i = cells.length - 1; i >= 0; i--) { > + // the cell that is our drop target is not a good choice Capital first latter, final full stop. ::: browser/base/content/newtab/grid.js @@ +112,5 @@ > + /** > + * Locks the grid to block all pointer events. > + */ > + lock: function Grid_lock() { > + this.node.setAttribute("locked", ""); Set the value to "true" - makes it look more deliberate whenever someone looks at the generated DOM. ::: browser/base/content/newtab/page.js @@ +57,5 @@ > + else > + this._setDisabled(true); > + > + // Register the preference observer. > + Services.prefs.addObserver(PREF_NEWTAB_ENABLED, this, true); This should probably be in the JSM (AllPages), so only one observer is ever added. Could cache the value of isEnabled there too, to save getting it from the prefs service every time. @@ +80,5 @@ > + /** > + * Checks if the page is modified and sets the CSS class accordingly > + */ > + checkIfModified: function Page_checkIfModified() { > + let classes = document.body.classList; This appears to be unused. @@ +141,5 @@ > + _setDisabled: function Page_setDisabled(aValue) { > + let nodes = [gGrid.node]; > + > + ["show", "hide", "reset"].forEach(function (aButton) { > + let id = "toolbar-button-" + aButton; Can you get these via a class instead? If an addon wants to add any buttons there, there's currently no easy way for them to have their buttons observe the disabled state. ::: browser/base/content/newtab/sites.js @@ +74,5 @@ > + > + /** > + * The site's parent cell. > + */ > + get cell() this.node.parentNode && this.node.parentNode.__newtabCell, This deserves some brackets. ::: browser/base/content/newtab/transformations.js @@ +123,5 @@ > + * unfreeze - unfreeze the site after rearranging > + * callback - the callback to call when finished > + */ > + rearrangeSites: > + function Transformation_rearrangeSites(aSites, aOptions) { This linebreak is unneeded (the line is less than 80 characters anyway). @@ +191,5 @@ > + */ > + _hideSite: function Transformation_hideSite(aSite, aCallback) { > + gAnimations.fadeOut(aSite.node, function () { > + aSite.node.setAttribute("hidden", ""); > + aCallback(); Need to null-check aCallback here, since rearrangeSites allows for it to be null.
Attachment #581451 -
Flags: review?(bmcbride) → review+
Comment 74•12 years ago
|
||
Comment on attachment 581436 [details] [diff] [review] Part 4 - Shared Module Review of attachment 581436 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/NewTabUtils.jsm @@ +85,5 @@ > + /** > + * Initializes the storage. > + */ > + init: function Storage_init() { > + Services.obs.addObserver(this._createPrivateBrowsingObserver(), Why do you need a function to generate this? Can't Storage just directly implement nsIObserver?
Attachment #581436 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 75•12 years ago
|
||
Removed two unnecessary about:newtab occurrences that enforced a blank tab title.
Attachment #581432 -
Attachment is obsolete: true
Attachment #581570 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 76•12 years ago
|
||
(In reply to Blair McBride (:Unfocused) from comment #74) > Why do you need a function to generate this? Can't Storage just directly > implement nsIObserver? I have no idea why I did this. Done, much cleaner now. (In reply to Marco Bonardo [:mak] from comment #71) > discussed on IRC, I think the PlacesAutocomplete overhead is excessive for > this task, while it could follow the same code path as Win 7 jumplists and > still get a frecency ordered list. Thanks for your advice! This is a lot better now without the need to extend nsPlacesAutoComplete. REDIRECTS_MODE_TARGET is just awesome (I thought there's now way to get a behavior like this).
Attachment #581428 -
Attachment is obsolete: true
Attachment #581436 -
Attachment is obsolete: true
Attachment #581579 -
Flags: review?(mak77)
Assignee | ||
Comment 77•12 years ago
|
||
Moved the preference observer to the JSM.
Attachment #581579 -
Attachment is obsolete: true
Attachment #581588 -
Flags: review?(mak77)
Attachment #581579 -
Flags: review?(mak77)
Assignee | ||
Comment 78•12 years ago
|
||
(In reply to Blair McBride (:Unfocused) from comment #73) > Global nit: Comments describing params/return values should be full > sentences too. Fixed. > > + _start: function CssAnimations_start() { > > + let node = this._node; > > + let name = this._name; > > + let classes = node.classList; > > + let callback = this._callback; > > You set this._node, this._name, and this._callback in the constructor, but > then just pull them out again here (which is only called by the > constructor). Should either directly use this._node etc (preferred, IMO), or > just pass them as parameters (or merge _start into the constructor, for that > matter). Fixed by merging the CssAnimations class into gAnimation. The code really didn't justify it being a single class, so it's now a single method instead. > > + let sites = gDropPreview.rearrange(aCell); > > + > > + // Filter out pinned sites. > > + let pinnedSites = sites.filter(function (aSite) { > > + return aSite && aSite.isPinned(); > > Can aSite ever be null? Seems like it should be guaranteed by rearrange() to > always be non-null. Yes it can, there can be empty cells in the grid which are returned as 'null' by Grid.sites. > > + _repositionPinnedSites: > > + function DropPreview_repositionPinnedSites(aSites, aCell) { > > Splitting it like this makes it look like a typo or an inner function. > > Reads easier if you split this line like so (even if it means breaking 80 > characters, IMO): > > _repositionPinnedSites: function DropPreview_repositionPinnedSites(aSites, > aCell) { Splitting lines like this is not uncommon (e.g. in SessionStore, Panorama). Is there a convention to follow for future code? > > + this.node.setAttribute("locked", ""); > > Set the value to "true" - makes it look more deliberate whenever someone > looks at the generated DOM. Fixed (for all state attributes). > > + Services.prefs.addObserver(PREF_NEWTAB_ENABLED, this, true); > > This should probably be in the JSM (AllPages), so only one observer is ever > added. Could cache the value of isEnabled there too, to save getting it from > the prefs service every time. Good point, moved to the JSM. > > + ["show", "hide", "reset"].forEach(function (aButton) { > > + let id = "toolbar-button-" + aButton; > > Can you get these via a class instead? If an addon wants to add any buttons > there, there's currently no easy way for them to have their buttons observe > the disabled state. Gosh, that looks horrible (I should've got some sleep). Fixed by querying the toolbar button class. > > + _hideSite: function Transformation_hideSite(aSite, aCallback) { > > + gAnimations.fadeOut(aSite.node, function () { > > + aSite.node.setAttribute("hidden", ""); > > + aCallback(); > > Need to null-check aCallback here, since rearrangeSites allows for it to be > null. Yes, good catch.
Attachment #581451 -
Attachment is obsolete: true
Attachment #581594 -
Flags: review?(dietrich)
Comment 79•12 years ago
|
||
Comment on attachment 581588 [details] [diff] [review] Part 4 - Shared Module Review of attachment 581588 [details] [diff] [review]: ----------------------------------------------------------------- I don't see anything blocking or problematic, just minor comments. ::: browser/base/content/NewTabUtils.jsm @@ +49,5 @@ > +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils", > + "resource://gre/modules/PlacesUtils.jsm"); > + > +// The preference that tells whether this feature is enabled. > +const PREF_NEWTAB_ENABLED = "browser.newtab.enabled"; I don't know if this was raised before, but the pref should be more like browser.newtabpage.enable imo, newtab is a far more generic concept. @@ +52,5 @@ > +// The preference that tells whether this feature is enabled. > +const PREF_NEWTAB_ENABLED = "browser.newtab.enabled"; > + > +// The maximum number of results we want to retrieve from the places history. > +const PLACES_RESULTS_LIMIT = 100; nit: s/places/history/ imo, so "retrieve from history" and HISTORY_RESULTS_LIMIT Places is just one of the possible history implementations, ideally. @@ +91,5 @@ > + /** > + * Initializes the storage. > + */ > + init: function Storage_init() { > + Services.obs.addObserver(this, "private-browsing-transition-complete", true); shouldn't this be in the storage lazy getter? why do we have to start listening before we have a storage? @@ +107,5 @@ > + try { > + value = JSON.parse(this.storage.getItem(aName)); > + } catch (e) {} > + > + return value || aDefault; you may avoid the local var by try { return JSON.parse... } catch(e) {} return aDefault; @@ +132,5 @@ > + * browsing mode changes. > + */ > + observe: function Storage_observe() { > + if (gPrivateBrowsing.privateBrowsingEnabled) { > + // When switching to private browsing mode we keep the current state according to your needs, you may rather listen to "private-browsing" and check if the data argument is "enter" or "exit", should be cheaper than checking privateBrowsingEnabled @@ +183,5 @@ > + getItem: function PrivateBrowsingStorage_getItem(aName) { > + if (aName in this._data) > + return this._data[aName]; > + > + return null; I think return this._data[aName] || null; would work and not warn @@ +253,5 @@ > + * value changes. > + */ > + observe: function AllPages_observe() { > + let enabled = Services.prefs.getBoolPref(PREF_NEWTAB_ENABLED); > + this._pages.forEach(function (aPage) aPage.setDisabled(!enabled)); Is there a reason you have setDisabled and not a disabled setter on a page? @@ +289,5 @@ > + pin: function PinnedLinks_pin(aLink, aIndex) { > + // Clear the link's old position, if any. > + this.unpin(aLink); > + > + this.links[aIndex] = aLink; Are pinned links a fixed list? I mean, this will replace whatever was at this position, rather than making space for a new entry. I assume this is the wanted behavior? @@ +300,5 @@ > + */ > + unpin: function PinnedLinks_unpin(aLink) { > + let index = this._indexOfLink(aLink); > + > + if (-1 < index) { nit: I'd prefer a index != -1 check, I was wondering what this was trying to check nit: the newline doesn't seem useful. @@ +331,5 @@ > + _indexOfLink: function PinnedLinks_indexOfLink(aLink) { > + for (let i = 0; i < this.links.length; i++) { > + let link = this.links[i]; > + > + if (link && link.url == aLink.url) nit: the newline doesn't seem useful. @@ +385,5 @@ > + * Checks whether the list of blocked links is empty. > + * @return Whether the list is empty. > + */ > + isEmpty: function BlockedLinks_isEmpty() { > + return !Object.keys(this.links).length; nit: == 0 is usually preferred for length checks @@ +399,5 @@ > + > +/** > + * Singleton that serves as the default link provider for the grid. It queries > + * the places history to retrieve the most frequently visited sites. > + */ ditto s/the places// @@ +421,5 @@ > + let callback = { > + handleResult: function (aResultSet) { > + let row; > + > + while (row = aResultSet.getNextRow()) { You can for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) @@ +470,5 @@ > + > + this._provider.getLinks(function (aLinks) { > + self._links = aLinks; > + aCallback && aCallback(); > + }); would bind work, instead of self? this._provider.getLinks((function (aLinks) {}).bind(this)); @@ +478,5 @@ > + * Gets the current set of links contained in the grid. > + * @return The links in the grid. > + */ > + getLinks: function Links_getLinks() { > + let pinnedLinks = PinnedLinks.links.concat(); If you are using concat to get a copy of the array, I think the usual way in the codebase is Array.slice(PinnedLinks.links); @@ +494,5 @@ > + // Append the remaining links if any. > + if (links.length) > + pinnedLinks = pinnedLinks.concat(links); > + > + return pinnedLinks; is there a reason PinnedLinks couldn't handle this internally by making space for new entries and compacting on removal?
Attachment #581588 -
Flags: review?(mak77) → review+
Updated•12 years ago
|
Whiteboard: [a11y-ping=davidb]
Comment 80•12 years ago
|
||
The two buttons on top right are again transparent on windows 7 with this recent patch on the latest UX tinderbox.
Assignee | ||
Comment 81•12 years ago
|
||
(In reply to scrapmachines from comment #80) > The two buttons on top right are again transparent on windows 7 with this > recent patch on the latest UX tinderbox. Sorry, just a stupid typo :( Will update the UX branch in some minutes.
Attachment #581441 -
Attachment is obsolete: true
Attachment #581715 -
Flags: review?(dao)
Attachment #581441 -
Flags: review?(dao)
Assignee | ||
Comment 82•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #79) > I don't know if this was raised before, but the pref should be more like > browser.newtabpage.enable imo, newtab is a far more generic concept. True, fixed. > > + init: function Storage_init() { > > + Services.obs.addObserver(this, "private-browsing-transition-complete", true); > > shouldn't this be in the storage lazy getter? why do we have to start > listening before we have a storage? Yeah, this can be added lazily, done. > you may avoid the local var by > try { > return JSON.parse... > } catch(e) {} > return aDefault; I didn't change that because I actually want a default value if getItem() returns null (which is the case when no value is set). > according to your needs, you may rather listen to "private-browsing" and > check if the data argument is "enter" or "exit", should be cheaper than > checking privateBrowsingEnabled Done. > > + let enabled = Services.prefs.getBoolPref(PREF_NEWTAB_ENABLED); > > + this._pages.forEach(function (aPage) aPage.setDisabled(!enabled)); > > Is there a reason you have setDisabled and not a disabled setter on a page? There was a Page.isEnabled() so I turned both of them into get/set enabled(). > Are pinned links a fixed list? I mean, this will replace whatever was at > this position, rather than making space for a new entry. I assume this is > the wanted behavior? Yes, they're a fixed list, additional to the list of links. Replacing the old entry is what we want. > > + // Append the remaining links if any. > > + if (links.length) > > + pinnedLinks = pinnedLinks.concat(links); > > + > > + return pinnedLinks; > > is there a reason PinnedLinks couldn't handle this internally by making > space for new entries and compacting on removal? This is the place where the list of pinned links is merged with the current list of links (excluding blocked links). So this is nothing that can be handled by PinnedLinks.
Attachment #581588 -
Attachment is obsolete: true
Assignee | ||
Comment 83•12 years ago
|
||
Combined Page.isEnabled() and Page.setDisabled() into get/set enabled().
Attachment #581427 -
Attachment is obsolete: true
Attachment #581730 -
Flags: review?(dietrich)
Attachment #581427 -
Flags: review?(dietrich)
Assignee | ||
Comment 84•12 years ago
|
||
Changed the preference name to browser.newtabpage.enabled.
Attachment #581570 -
Attachment is obsolete: true
Attachment #581731 -
Flags: review?(gavin.sharp)
Attachment #581570 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 85•12 years ago
|
||
Combined Page.isEnabled() and Page.setDisabled() into get/set enabled(). Changed the preference name to browser.newtabpage.enabled.
Attachment #581594 -
Attachment is obsolete: true
Attachment #581733 -
Flags: review?(dietrich)
Attachment #581594 -
Flags: review?(dietrich)
Comment 86•12 years ago
|
||
Comment on attachment 581731 [details] [diff] [review] Part 3 - about:newtab integration >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js >+// Enable the New Tab Page (about:newtab) >+pref("browser.newtabpage.enabled", true); Where does this pref actually get read? I see an observer in the module in the other patch, but it just sets "enabled" properties on elements in _pages, and I don't see where this has any effect. Shouldn't toggling this pref result in us reverting to using about:blank for new tabs? >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >- "chrome,all,dialog=no", "about:blank"); >+ "chrome,all,dialog=no", "about:newtab"); >- gBrowser.loadOneTab("about:blank", {inBackground: false}); >+ gBrowser.loadOneTab("about:newtab", {inBackground: false}); How about adding a |const BROWSER_NEW_TAB_URL = "about:newtab";| in browser.js, and using it in these two places as well as the addTab call in tabbrowser.xml? If you want the pref to impact this, you could also make it a lazy getter. >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >- if (!this.mBlank) { >+ if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") { What was wrong with my suggestion of changing the definition of "blank" (and therefore mBlank) to include about:newtab?
Assignee | ||
Comment 87•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86) > Where does this pref actually get read? I see an observer in the module in > the other patch, but it just sets "enabled" properties on elements in > _pages, and I don't see where this has any effect. Shouldn't toggling this > pref result in us reverting to using about:blank for new tabs? It's accessed in page.js (part 1). toolbar.js (part 1) sets this pref so that all pages get notified of this change. > >- "chrome,all,dialog=no", "about:blank"); > >+ "chrome,all,dialog=no", "about:newtab"); > > >- gBrowser.loadOneTab("about:blank", {inBackground: false}); > >+ gBrowser.loadOneTab("about:newtab", {inBackground: false}); > > How about adding a |const BROWSER_NEW_TAB_URL = "about:newtab";| in > browser.js, and using it in these two places as well as the addTab call in > tabbrowser.xml? If you want the pref to impact this, you could also make it > a lazy getter. Will do that. > >- if (!this.mBlank) { > >+ if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") { > > What was wrong with my suggestion of changing the definition of "blank" (and > therefore mBlank) to include about:newtab? As Frank said: (In reply to Frank Yan (:fryn) from comment #69) > The problem is that `blank` is also used for other things that should not > apply to about:newtab (e.g. when `blank` is true, we don't bother to stop > the about:blank load in anticipation of another document being loaded > immediately afterward). The problem with mBlank is that the progress listener sets |mBlank = false| after the first STATE_STOP so that doesn't work either with about:newtab and its images (the throbber will be turned on with the first resource that is loaded).
Assignee | ||
Comment 88•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86) > Shouldn't toggling this > pref result in us reverting to using about:blank for new tabs? No, we'll have a blank page without the grid that has a tiny icon in the upper right corner to easily re-enable the 'New Tab Page'. The whole page won't be initialized (but of course parsed) if it's pref'ed off.
Comment 89•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #88) > (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #86) > > Shouldn't toggling this > > pref result in us reverting to using about:blank for new tabs? > > No, we'll have a blank page without the grid that has a tiny icon in the > upper right corner to easily re-enable the 'New Tab Page'. The whole page > won't be initialized (but of course parsed) if it's pref'ed off. Ah, I somewhat misunderstood this too. I guess the following comment in the code should better explain that it simply toggles the content of the page, not the usage of about:newtab vs. about:blank. >+// Enable the New Tab Page (about:newtab) >+pref("browser.newtabpage.enabled", true);
Assignee | ||
Comment 90•12 years ago
|
||
(In reply to Frank Yan (:fryn) from comment #89) > Ah, I somewhat misunderstood this too. I guess the following comment in the > code should better explain that it simply toggles the content of the page, > not the usage of about:newtab vs. about:blank. Done. (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86) > How about adding a |const BROWSER_NEW_TAB_URL = "about:newtab";| in > browser.js, and using it in these two places as well as the addTab call in > tabbrowser.xml? Done. > >- if (!this.mBlank) { > >+ if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") { > > What was wrong with my suggestion of changing the definition of "blank" (and > therefore mBlank) to include about:newtab? I didn't change this, yet. Is there a better way of doing this? Should we add another argument? Should we prevent setting mBlank to false?
Attachment #581731 -
Attachment is obsolete: true
Attachment #581760 -
Flags: review?(gavin.sharp)
Attachment #581731 -
Flags: review?(gavin.sharp)
Comment 91•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #78) > Splitting lines like this is not uncommon (e.g. in SessionStore, Panorama). > Is there a convention to follow for future code? Maybe it's just uncommon/not used in the code I usually touch. I don't think we have any formal convention for that... I just find one style easier to read/understand-at-a-glance than the other.
Comment 92•12 years ago
|
||
Can you set BROWSER_NEW_TAB_URL to about:blank depending on the pref?
Assignee | ||
Comment 93•12 years ago
|
||
How about introducing a second pref? So there's one pref to toggle this feature (temporary) via the button in about:newtab. The second pref (not exposed via UI) switches back to about:blank like you said to satisfy people not wanting it at all. That should of course prevent the whole module from loading and initializing.
Comment 94•12 years ago
|
||
I'm thinking the primary use case for reverting to about:blank would be to turn off the feature on aurora if needed. Also, if that pref is a string, add-ons can utilize it.
Assignee | ||
Comment 95•12 years ago
|
||
Ok, that sounds like a good idea. So we'll just introduce a new pref for a new tab's url. So it's easy to pref it off on aurora and extensible.
Comment 96•12 years ago
|
||
Comment on attachment 581715 [details] [diff] [review] Part 2 - Assets / CSS / Images Please move aesthetic / non-functional styling to the theme css files. Get rid of the descendent selectors; for body[dir=rtl], follow bug 700036. Break the line after each comma in selectors. Why are you using -moz-animation-* instead of -moz-transition?
Attachment #581715 -
Flags: review?(dao) → review-
Assignee | ||
Comment 97•12 years ago
|
||
Introduced the pref 'browser.newtab.url'. Discovered some additional places where we should use BROWSER_NEW_TAB_URL instead of 'about:blank' to get a consistent new tab behavior.
Attachment #581760 -
Attachment is obsolete: true
Attachment #582058 -
Flags: review?(gavin.sharp)
Attachment #581760 -
Flags: review?(gavin.sharp)
Comment 98•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #90) > > What was wrong with my suggestion of changing the definition of "blank" (and > > therefore mBlank) to include about:newtab? > > I didn't change this, yet. Is there a better way of doing this? Should we > add another argument? Should we prevent setting mBlank to false? I was suggesting changing the value of the "var blank" in addTab() to be true if the URL is about:newtab. That variable gets passed to mTabProgressListener as mBlank, so changing it there should make your other tabbrowser change unnecessary. I haven't looked closely into what other side effects that may have, but it seems to make sense conceptually.
Comment 99•12 years ago
|
||
Comment on attachment 582058 [details] [diff] [review] Part 3 - about:newtab integration >+__defineGetter__("BROWSER_NEW_TAB_URL", function() { >+ return gPrefService.getCharPref("browser.newtab.url", "about:blank"); >+}); I don't think the pref needs to be live. If we wanted to support this, an observer would be the way to go.
Assignee | ||
Comment 100•12 years ago
|
||
Oops. Yeah, I also don't think it needs to be live. I actually meant to implement a lazy getter...
Assignee | ||
Comment 101•12 years ago
|
||
Limi asked me to remove the history dropmarker from the URL bar as a part of this patch because that's needed to avoid UI redundancy. Removing the history dropmarker was one side-goal of the New Tab Page.
Attachment #582241 -
Flags: review?(dao)
Comment 102•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #101) > Created attachment 582241 [details] [diff] [review] > Part 6 - Remove the history dropmarker from the URL bar This is again an unnecessary change. Removing something which has been present for years, without any major gains, appears to be meaningless.
Assignee | ||
Comment 103•12 years ago
|
||
Removed gAnimation/animations.js because that's actually legacy code and we can achieve the same with CSS transformations. Rewrote the New Tab Page to be a XUL file, but using the HTML namespace as default. That enables us to use the :-moz-local-dir(rtl) selector and avoid descendant selectors.
Attachment #581733 -
Attachment is obsolete: true
Attachment #582247 -
Flags: review?(dietrich)
Attachment #581733 -
Flags: review?(dietrich)
Assignee | ||
Comment 104•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #96) > Please move aesthetic / non-functional styling to the theme css files. Done. > Get rid of the descendent selectors; for body[dir=rtl], follow bug 700036. Done. > Why are you using -moz-animation-* instead of -moz-transition? That was some legacy code. I removed the CSS animations and switched to using CSS transitions.
Attachment #581715 -
Attachment is obsolete: true
Attachment #582251 -
Flags: review?(dao)
Comment 105•12 years ago
|
||
I have found a bug with the version of this patch that is pushed to the UX branch. STR: 1. Maximize the window 2. Open a new tab 3. Try to tear off the tab to make it a new window. Actual results: The tab will not tear off. Even more odd, is that if the tab is dragged on top of the grid of sites, the grid will make space for the tab and if the tab is dropped on the grid then the "New Tab page" tab will become part of the grid. Expected results: The tab is torn off and creates a new window.
Comment 106•12 years ago
|
||
(In reply to Jared Wein [:jwein and :jaws] from comment #105) > I have found a bug with the version of this patch that is pushed to the UX > branch. > > STR: > 1. Maximize the window > 2. Open a new tab > 3. Try to tear off the tab to make it a new window. > > Actual results: The tab will not tear off. Even more odd, is that if the tab > is dragged on top of the grid of sites, the grid will make space for the tab > and if the tab is dropped on the grid then the "New Tab page" tab will > become part of the grid. > > Expected results: The tab is torn off and creates a new window. Works fine for me on build with changeset f0f24e6d7587 . The New Tab tears off well and works like any other torn off tab. Although the New Tab can be added in the New Tab page's Tiles which then becomes a blank Tile with no link. Just a white Rectangle. This should not happen. New Tab page should not allow drop of New Tab page itself on the Tiles.
Assignee | ||
Comment 107•12 years ago
|
||
(In reply to Jared Wein [:jwein and :jaws] from comment #105) > Actual results: The tab will not tear off. Even more odd, is that if the tab > is dragged on top of the grid of sites, the grid will make space for the tab > and if the tab is dropped on the grid then the "New Tab page" tab will > become part of the grid. Thanks for reporting this! I didn't check the dataTransfer type when accepting drops, fixed. I'll update the patch on the UX branch later today.
Attachment #582247 -
Attachment is obsolete: true
Attachment #582663 -
Flags: review?(dietrich)
Attachment #582247 -
Flags: review?(dietrich)
Assignee | ||
Comment 108•12 years ago
|
||
Fixed a bug in isTabEmpty(). BROWSER_NEW_TAB_URL is now a lazy getter.
Attachment #582058 -
Attachment is obsolete: true
Attachment #582901 -
Flags: review?(gavin.sharp)
Attachment #582058 -
Flags: review?(gavin.sharp)
Comment 109•12 years ago
|
||
Comment on attachment 582901 [details] [diff] [review] Part 3 - about:newtab integration >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > function isTabEmpty(aTab) { >- !browser.contentDocument.body.hasChildNodes() && >- !aTab.hasAttribute("busy"); >+ (!body || !body.hasChildNodes()) && !aTab.hasAttribute("busy"); Were you actually hitting body being null here, or are you just being cautious? nit: leaving the hasAttribute check on its own line makes this easier to read, IMO. >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >- if (!this.mBlank) { >+ if (!this.mBlank && this.mBrowser.userTypedValue != BROWSER_NEW_TAB_URL) { I still think we should investigate modifying mBlank to avoid the need for a specific userTypedValue check here, but we can do that in a followup bug I guess.
Attachment #582901 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 110•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #109) > Were you actually hitting body being null here, or are you just being > cautious? I actually hit that while testing locally. The New Tab Page seems to have no document.body so that threw here and I added a check for that. > I still think we should investigate modifying mBlank to avoid the need for a > specific userTypedValue check here, but we can do that in a followup bug I > guess. Ok, I'll file a bug.
Comment 111•12 years ago
|
||
Comment on attachment 582663 [details] [diff] [review] Part 1 - XHTML Page / Scripts Review of attachment 582663 [details] [diff] [review]: ----------------------------------------------------------------- overall comment: i like that things are broken up into separate include files. however the fact that the split-out code relies on globals (gGid, gDrag, etc) is confusing, since it's not clear where those are defined and managed. i'd prefer that those globals be passed into object/method scope via ctor, individually or via a configuration object, rather than having the split-out code use them directly. however, that can be done as a followup refactoring since it's mostly just be variable renaming. r=me with the noted changes made, and questions answered. ::: browser/base/content/newtab/cells.js @@ +41,5 @@ > + * This class represents a cell contained in the grid. > + */ > +function Cell(aNode) { > + this._node = aNode; > + this._node.__newtabCell = this; what's the double underscore about? @@ +61,5 @@ > + /** > + * The cell's offset in the grid. > + */ > + get index() { > + let index = gGrid.cells.indexOf(this); can you pass the grid into the ctor instead of using a global? ::: browser/base/content/newtab/helper.js @@ +38,5 @@ > +#endif > + > +/** > + * This class makes it easy to wait until a batch of callbacks has finished. > + */ kind of weird that this is called helper.js but only has a single api for batching. maybe should call it batch.js? wrt the api itself: * should add an example * seems confusing that it doesn't handle the contents of the batch, but maybe that's just me ::: browser/base/content/newtab/sites.js @@ +40,5 @@ > +/** > + * This class represents a site that is contained in a cell and can be pinned, > + * moved around or deleted. > + */ > +function Site(aNode, aLink) { nit: rename file to site.js? @@ +80,5 @@ > + return parentNode && parentNode.__newtabCell; > + }, > + > + /** > + * Pins the site on it's current or a given index. nit: no apostrophe @@ +168,5 @@ > + * Adds event handlers for the site and its buttons. > + */ > + _addEventHandlers: function Site_addEventHandlers() { > + // Register drag-and-drop event handlers. > + ["DragStart", /*"Drag",*/ "DragEnd"].forEach(function (aType) { why the commented out bit? ::: browser/base/content/newtab/toolbar.js @@ +63,5 @@ > + /** > + * Enables the 'New Tab Page' feature. > + */ > + show: function Toolbar_show() { > + Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, true); i'd prefer have the enable/disable implementation centralized somewhere (page.js?) and then toggled via an api. for example, this code would call page.enable/disable() or toggle a boolean property there.
Attachment #582663 -
Flags: review?(dietrich) → review+
Comment 112•12 years ago
|
||
i see the grid on new tab also when browser.newtabpage.enabled is false
Comment 113•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #101) > Part 6 - Remove the history dropmarker from the URL bar We should move this to another bug.
Updated•12 years ago
|
Attachment #582241 -
Attachment is obsolete: true
Attachment #582241 -
Flags: review?(dao)
Assignee | ||
Comment 114•12 years ago
|
||
Sorry, have to ask for review again. BrowserOpenTab() recently changed and this revealed that we need to fix utilityOverlay.js as well.
Attachment #582901 -
Attachment is obsolete: true
Attachment #583448 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 115•12 years ago
|
||
Comment on attachment 583448 [details] [diff] [review] Part 3 - about:newtab integration Fixing some further integration bugs reported by the UX team (blinking 'about:newtab' in the location bar and similar stuff). Cancelling review for now.
Attachment #583448 -
Flags: review?(gavin.sharp)
Comment 116•12 years ago
|
||
The new tab page needs scrollbars, both vertical and horizontal. Another option could be to resize thumbnails as we do in Panorama.
Comment 117•12 years ago
|
||
Comment on attachment 582251 [details] [diff] [review] Part 2 - Assets / CSS / Images >+++ b/browser/base/content/newtab/newTab.css >+body { >+ position: relative; >+#toolbar { >+ position: absolute; If you use "fixed" here, you can remove "position: relative" from the body. >+ top: 8px; >+ right: 8px; should be in themes >+ list-style-type: none; What is this doing here? >+.site { >+ outline: none; >+ -moz-transition: 200ms ease-out; >+ -moz-transition-property: top, left, box-shadow, opacity; This looks like it should be in themes as well. You don't seem to set box-shadow here, for instance. >+.site:hover .site-img, >+.site[ontop] .site-img { >+.site:hover:not([frozen]) .site-strip { child selector >+.drag-element { >+ width: 1px; >+ height: 1px; >+ background-color: #fff; >+ opacity: 0.01; >+} More theme stuff? Not sure what this is doing, exactly. Maybe add a comment. >diff --git a/browser/base/content/newtab/strip.png b/browser/base/content/newtab/strip.png belongs in themes >--- /dev/null >+++ b/browser/locales/en-US/chrome/browser/newTab.dtd >@@ -0,0 +1,7 @@ >+<!ENTITY % brandDTD >+ SYSTEM "chrome://branding/locale/brand.dtd"> >+ %brandDTD; >+ >+<!-- These strings are used in the about:newtab page --> >+ >+<!ENTITY newtab.pageTitle "New Tab"> I think a couple of things on this page need tooltips. Speaking of which, has a11y been reviewed? Oh, and yes, some sensible overflow behavior would be nice.
Attachment #582251 -
Flags: review?(dao)
Assignee | ||
Comment 118•12 years ago
|
||
1) Introduced isNewTabURL() to get rid of all the recurring (url == 'about:blank' || url == BROWSER_NEW_TAB_URL) checks. 2) Some a11y tests do crazy stuff with the tabbrowser and yield that isNewTabURL() isn't defined so I moved the method to gBrowser.isNewTabURL() and replaced BROWSER_NEW_TAB_URL with gBrowser.newTabURL instead of having them defined in browser.js. 3) After the initial 'about:blank' load was stopped the patch now re-sets mBlank=true because that has been set to false by STATUS_STOP when tabbrowser calls browser.stop() before loading the actual URI. So we can now re-use the 'blank' variable for the mTabProgressListener. 4) Replaced 'about:blank' occurrences in browser-places.js and browser-fullZoom.js with gBrowser.isNewTabURL().
Attachment #583448 -
Attachment is obsolete: true
Attachment #583802 -
Flags: review?(gavin.sharp)
Comment 119•12 years ago
|
||
Comment on attachment 583802 [details] [diff] [review] Part 3 - about:newtab integration utilityOverlay.js can't always access gBrowser and it's not always loaded with browser.js either, which means that BROWSER_NEW_TAB_URL / isNewTabURL() need to be defined there. Please don't let those crazy a11y tests dictate the implementation. You can define dummy functions there or let them include utilityOverlay.js or do whatever local hack it takes to get them pass.
Attachment #583802 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 120•12 years ago
|
||
Is it okay to define isNewTabURL() and BROWSER_NEW_TAB_URL in utilityOverlay.js? Can tabbrowser.xml assume that this is loaded? Does the tabbrowser need to define its own functions to stay an independent XUL element or does it already have dependencies like this?
Comment 121•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #120) > Is it okay to define isNewTabURL() and BROWSER_NEW_TAB_URL in > utilityOverlay.js? Can tabbrowser.xml assume that this is loaded? Yes. > Does the > tabbrowser need to define its own functions to stay an independent XUL > element or does it already have dependencies like this? It already has similar dependencies and we had to fix up these tests before.
Assignee | ||
Comment 122•12 years ago
|
||
Moved BROWSER_NEW_TAB_URL and isNewTabURL() to utilityOverlay.js. Fixed a11y tests by including utilityOverlay.js.
Attachment #583802 -
Attachment is obsolete: true
Attachment #583972 -
Flags: review?(gavin.sharp)
Comment 123•12 years ago
|
||
Comment on attachment 583972 [details] [diff] [review] Part 3 - about:newtab integration >+ return Services.prefs.getCharPref("browser.newtab.url", "about:blank"); getCharPref doesn't take a second argument.
Comment 125•12 years ago
|
||
Devs -- you might like to have a look at bug 713461 for more ideas.
Assignee | ||
Comment 126•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #123) > >+ return Services.prefs.getCharPref("browser.newtab.url", "about:blank"); > getCharPref doesn't take a second argument. Sorry, expected a default argument. Fixed.
Attachment #583972 -
Attachment is obsolete: true
Attachment #584434 -
Flags: review?(gavin.sharp)
Attachment #583972 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 127•12 years ago
|
||
Fixed a test-only mem leak.
Attachment #581730 -
Attachment is obsolete: true
Attachment #584436 -
Flags: review?(dietrich)
Attachment #581730 -
Flags: review?(dietrich)
Comment 128•12 years ago
|
||
Comment on attachment 584436 [details] [diff] [review] Part 5 - New Tab Page tests and test suite Review of attachment 584436 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/newtab/Makefile.in @@ +44,5 @@ > +include $(DEPTH)/config/autoconf.mk > +include $(topsrcdir)/config/rules.mk > + > +_BROWSER_FILES = \ > + browser_newtab_block.js \ nit: these should line up with something. ::: browser/base/content/test/newtab/head.js @@ +228,5 @@ > + > +/** > + * Simulates a drop and drop operation. > + * @param aDropTarget the cell that is the drop target > + * @param aDragSource the cell that contains the dragged site (optional) are these nsISomethings? please add actual type to this and to pin/blockCell as well.
Attachment #584436 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 129•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #128) > > +_BROWSER_FILES = \ > > + browser_newtab_block.js \ > > nit: these should line up with something. Fixed. > > + * Simulates a drop and drop operation. > > + * @param aDropTarget the cell that is the drop target > > + * @param aDragSource the cell that contains the dragged site (optional) > > are these nsISomethings? please add actual type to this and to pin/blockCell > as well. These are objects (instanceof Cell) custom to the new tab page.
Attachment #584436 -
Attachment is obsolete: true
Assignee | ||
Comment 130•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #111) > i'd prefer that those globals be passed into object/method scope via ctor, > individually or via a configuration object, rather than having the split-out > code use them directly. however, that can be done as a followup refactoring > since it's mostly just be variable renaming. Ok, I'll file a follow-up bug for this. > > +function Cell(aNode) { > > + this._node = aNode; > > + this._node.__newtabCell = this; > > what's the double underscore about? I use underscores when attaching JS objects to DOM nodes. That's a convention we use in Panorama (and SS iirc) as well. > > + get index() { > > + let index = gGrid.cells.indexOf(this); > > can you pass the grid into the ctor instead of using a global? Done. > kind of weird that this is called helper.js but only has a single api for > batching. maybe should call it batch.js? Renamed. > wrt the api itself: > > * should add an example Added. > * seems confusing that it doesn't handle the contents of the batch, but > maybe that's just me What exactly do you mean with "contents"? Basically this class just handles a collection of callbacks. > > +function Site(aNode, aLink) { > > nit: rename file to site.js? I'm not totally objecting this but I as the files are named 'sites.js' and 'cells.js' this says that these are not singletons but there will be many of them created. > > + _addEventHandlers: function Site_addEventHandlers() { > > + // Register drag-and-drop event handlers. > > + ["DragStart", /*"Drag",*/ "DragEnd"].forEach(function (aType) { > > why the commented out bit? This is related to bug 505521. Once that is implemented we could use the coordinates from the drag event. For the time being we need to use a workaround (which I noted in some comments) and therefore I commented it out. > > + show: function Toolbar_show() { > > + Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, true); > > i'd prefer have the enable/disable implementation centralized somewhere > (page.js?) and then toggled via an api. for example, this code would call > page.enable/disable() or toggle a boolean property there. Yeah that's a good point. I moved all the pref toggling and reading to the JSM. The AllPages object does now implement a 'enabled' getter/setter.
Attachment #582663 -
Attachment is obsolete: true
Attachment #585394 -
Flags: review?(dietrich)
Assignee | ||
Comment 131•12 years ago
|
||
Asking for review wrt to the changes of the AllPages object.
Attachment #581729 -
Attachment is obsolete: true
Attachment #585395 -
Flags: review?(dietrich)
Assignee | ||
Comment 132•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #117) > >+body { > >+ position: relative; > > >+#toolbar { > >+ position: absolute; > > If you use "fixed" here, you can remove "position: relative" from the body. Because of the HTML/XUL mix here 'position:absolute' doesn't really work for elements without the body having 'position:relative'. This also breaks drag and drop completely. Maybe it's because the layout engine doesn't really find a root element to position to? > >+ top: 8px; > >+ right: 8px; > > should be in themes Moved. > >+ list-style-type: none; > > What is this doing here? The grid with its sites/thumbnails is a list (ul/li). Without this some ugly bullets appear. > >+.site { > > >+ outline: none; > >+ -moz-transition: 200ms ease-out; > >+ -moz-transition-property: top, left, box-shadow, opacity; > > This looks like it should be in themes as well. You don't seem to set > box-shadow here, for instance. Moved. > >+.site:hover .site-img, > >+.site[ontop] .site-img { > > >+.site:hover:not([frozen]) .site-strip { > > child selector Done. > >+.drag-element { > >+ width: 1px; > >+ height: 1px; > >+ background-color: #fff; > >+ opacity: 0.01; > >+} > > More theme stuff? Not sure what this is doing, exactly. Maybe add a comment. Added a comment. > >diff --git a/browser/base/content/newtab/strip.png b/browser/base/content/newtab/strip.png > > belongs in themes Moved. > I think a couple of things on this page need tooltips. Added tooltips for all buttons. > Speaking of which, has a11y been reviewed? No, we want to do this in a follow-up bug (a11y and ARIA). This whole feature will land pref'ed off until we think it's 'shippable' (i.e. all important follow-up bugs are fixed). > Oh, and yes, some sensible overflow behavior would be nice. Indeed, that unfortunately got lost with the XHTML -> XUL transition... Fixed.
Attachment #582251 -
Attachment is obsolete: true
Attachment #585398 -
Flags: review?(dao)
Comment 133•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #130) > > > +function Cell(aNode) { > > > + this._node = aNode; > > > + this._node.__newtabCell = this; > > > > what's the double underscore about? > > I use underscores when attaching JS objects to DOM nodes. That's a > convention we use in Panorama (and SS iirc) as well. We usually use two underscores when tacking data to external objects that your code doesn't really control. For objects you own, a single underscore suffices.
Assignee | ||
Comment 134•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #133) > We usually use two underscores when tacking data to external objects that > your code doesn't really control. For objects you own, a single underscore > suffices. Ok, thanks for clearing this up! I replaced the double underscores with single ones.
Attachment #585394 -
Attachment is obsolete: true
Attachment #585454 -
Flags: review?(dietrich)
Attachment #585394 -
Flags: review?(dietrich)
Comment 135•12 years ago
|
||
Comment on attachment 585454 [details] [diff] [review] Part 1 - XUL Page / Scripts Review of attachment 585454 [details] [diff] [review]: ----------------------------------------------------------------- mostly just naming and comment changes for clarity. r=me with these fixes, given that there aren't any major changes involved. ::: browser/base/content/newtab/cells.js @@ +37,5 @@ > + * ***** END LICENSE BLOCK ***** */ > +#endif > + > +/** > + * This class represents a cell contained in the grid. should clarify that it manages the dom node for the cell, not the actual cell content (ie, the Site). might also want to note that it's basically a read-only class. all manipulation of both position and content happens elsewhere. @@ +46,5 @@ > + this._node._newtabCell = this; > + > + // Register drag-and-drop event handlers. > + ["DragEnter", "DragOver", "DragExit", "Drop"].forEach(function (aType) { > + let method = "_on" + aType; why are these prefixed with underscore? they're called from external code, so aren't really private. ::: browser/base/content/newtab/drag.js @@ +37,5 @@ > + * ***** END LICENSE BLOCK ***** */ > +#endif > + > +/** > + * This singleton implements site dragging functionality. should this singleton work at the level of cells, not sites? not sure why this code needs to be aware of a cell's contents - looking at it, it's all about the actual container itself. drop.js handles cells in the drop code, but sites in the drag code. can this be made consistent, or is that not possible for some reason? ::: browser/base/content/newtab/grid.js @@ +82,5 @@ > + > + /** > + * Creates a new site in the grid. > + * @param aLink The new site's link. > + * @param aCell The new site's parent cell. when you say 'parent', you mean it's the cell that the site will be created in, right? maybe 'containing' would be better than 'parent'? parent connotes the ability to have multiple children. where does the cell come from? what happens as a result of this? is an existing cell pushed off the grid? should document the side-effects. ::: browser/base/content/newtab/newTab.js @@ +43,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/Geometry.jsm"); > +Cu.import("resource:///modules/PageThumbs.jsm"); > +Cu.import("resource:///modules/NewTabUtils.jsm"); worth lazy-loading these? ::: browser/base/content/newtab/page.js @@ +87,5 @@ > + > + /** > + * Checks if the page is modified and sets the CSS class accordingly > + */ > + checkIfModified: function Page_checkIfModified() { the method name doesn't indicate any side-effects. this doesn't just check, it sets a value as well. maybe updateModifiedFlag or something ungainly like that? @@ +116,5 @@ > + > + // Workaround to prevent a delay on MacOSX due to a slow drop animation. > + let doc = document.documentElement; > + doc.addEventListener("dragover", this._onDragOver, false); > + doc.addEventListener("drop", this._onDrop, false); these aren't really private, so shouldn't be underscore prefixed. ::: browser/base/content/newtab/transformations.js @@ +37,5 @@ > + * ***** END LICENSE BLOCK ***** */ > +#endif > + > +/** > + * This singleton allows to transform the grid by moving sites around. should say whether it works on sites, cells, site-nodes or cell-nodes, or a mix of the above. ::: browser/base/content/newtab/updater.js @@ +79,5 @@ > + * their positions). > + * @param aLinks The array of links contained in the current grid. > + * @return The remaining sites. > + */ > + _findRemainingSites: function Updater_findRemainingSites(aLinks) { can you clarify the name and comment to say what these are remaining from? what's the datatype of aLinks? what's the datatype of the return value? @@ +111,5 @@ > + * @param aSites The array of sites to move. > + */ > + _moveSiteNodes: function Updater_moveSiteNodes(aSites) { > + let cells = gGrid.cells; > + let sites = aSites.slice(0, cells.length); imagine i'm a dev looking at this code for the first time, trying to fix a bug or add a feature... why does this need to happen? can you add some more commentary to explain the context of this call?
Attachment #585454 -
Flags: review?(dietrich) → review+
Comment 136•12 years ago
|
||
Comment on attachment 585395 [details] [diff] [review] Part 4 - Shared Module Review of attachment 585395 [details] [diff] [review]: ----------------------------------------------------------------- f+. let's get these fixes done and questions answered, then i'll do another pass. ::: browser/modules/NewTabUtils.jsm @@ +54,5 @@ > + > +// The maximum number of results we want to retrieve from history. > +const HISTORY_RESULTS_LIMIT = 100; > + > +// Define some lazyily loaded services. typo @@ +62,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "gScriptSecurityManager", > + "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "gStorageManager", > + "@mozilla.org/dom/storagemanager;1", "nsIDOMStorageManager"); move all these lazy getters to services.jsm? (iirc pb is already there) @@ +92,5 @@ > + if (gPrivateBrowsing.privateBrowsingEnabled) > + this._storage = new PrivateBrowsingStorage(this._storage); > + > + // Register an observer to listen for private browsing mode changes. > + if (!this._observing) { since this whole code block will only ever be called once (because _storage gets set above), this is unnecessary right? also, instead of having these |if| blocks and saving _storage and _observing, could instead memo-ize the whole getter, eg: get storage() { let storage = ...; this.__defineGetter__('storage', storage); } update: nm, i see below in the pb listener that you're blowing away the private vars. i don't like how this is all set up, seems more complex that it should be. that said, i don't see a much clearer way at the moment :( @@ +122,5 @@ > + * Sets the storage value for a given key. > + * @param aName The storage key. > + * @param aValue The value to set. > + */ > + set: function Storage_set(aName, aValue) { any kind of key validation need to be done here? @@ +210,5 @@ > + > +/** > + * Singleton that serves as a registry for all open 'New Tab Page's. > + */ > +let AllPages = { thought for a followup - any reason to not just create a single hidden page, and swap it into view as needed, the way that we do for jetpack panel/widget content? @@ +440,5 @@ > + /** > + * Gets the current set of links delivered by this provider. > + * @param aCallback The callback to call when the links have been retrieved. > + */ > + getLinks: function PlacesProvider_getLinks(aCallback) { should document the datatype of what's passed to aCallback. should also document what you mean by 'link' in the Links singleton below.
Attachment #585395 -
Flags: review?(dietrich) → feedback+
Comment 137•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #136) > @@ +210,5 @@ > > + > > +/** > > + * Singleton that serves as a registry for all open 'New Tab Page's. > > + */ > > +let AllPages = { > > thought for a followup - any reason to not just create a single hidden page, > and swap it into view as needed, the way that we do for jetpack panel/widget > content? Hm, related: bug 715612. Imagine when I restore my 364 tab session. We should look into not actually doing anything for hidden tabs. Could either show new-tab-page content on-demand in those cases, or just fix it as above.
Assignee | ||
Comment 138•12 years ago
|
||
Not sure how easy it would be to swap a single page to the actively shown tab. We can't just swap the docShells because that would include the whole tab's history. Restoring your big session shouldn't be a problem. SS creates 'about:blank' tabs (not about:newtab) and then waits for them to actually be restored.
Assignee | ||
Comment 139•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #136) > > +XPCOMUtils.defineLazyServiceGetter(this, "gScriptSecurityManager", > > + "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager"); > > + > > +XPCOMUtils.defineLazyServiceGetter(this, "gStorageManager", > > + "@mozilla.org/dom/storagemanager;1", "nsIDOMStorageManager"); > > move all these lazy getters to services.jsm? (iirc pb is already there) Moved. > > + if (gPrivateBrowsing.privateBrowsingEnabled) > > + this._storage = new PrivateBrowsingStorage(this._storage); > > + > > + // Register an observer to listen for private browsing mode changes. > > + if (!this._observing) { > > since this whole code block will only ever be called once (because _storage > gets set above), this is unnecessary right? > > also, instead of having these |if| blocks and saving _storage and > _observing, could instead memo-ize the whole getter, eg: > > get storage() { > let storage = ...; > this.__defineGetter__('storage', storage); > } > > update: nm, i see below in the pb listener that you're blowing away the > private vars. i don't like how this is all set up, seems more complex that > it should be. that said, i don't see a much clearer way at the moment :( I split it into .domStorage and .currentStorage. Looks a bit cleaner now IMO. > > + * Sets the storage value for a given key. > > + * @param aName The storage key. > > + * @param aValue The value to set. > > + */ > > + set: function Storage_set(aName, aValue) { > > any kind of key validation need to be done here? The key must be a string but as this internal API is only used by the module itself, should we really check its type? I added that we're expecting a string here in the @param comment. > > +/** > > + * Singleton that serves as a registry for all open 'New Tab Page's. > > + */ > > +let AllPages = { > > thought for a followup - any reason to not just create a single hidden page, > and swap it into view as needed, the way that we do for jetpack panel/widget > content? Please see comment #138. > > + /** > > + * Gets the current set of links delivered by this provider. > > + * @param aCallback The callback to call when the links have been retrieved. > > + */ > > + getLinks: function PlacesProvider_getLinks(aCallback) { > > should document the datatype of what's passed to aCallback. should also > document what you mean by 'link' in the Links singleton below. Done.
Attachment #585395 -
Attachment is obsolete: true
Attachment #586278 -
Flags: review?(dietrich)
Assignee | ||
Comment 140•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #135) > > +/** > > + * This class represents a cell contained in the grid. > > should clarify that it manages the dom node for the cell, not the actual > cell content (ie, the Site). > > might also want to note that it's basically a read-only class. all > manipulation of both position and content happens elsewhere. Done. > > + // Register drag-and-drop event handlers. > > + ["DragEnter", "DragOver", "DragExit", "Drop"].forEach(function (aType) { > > + let method = "_on" + aType; > > why are these prefixed with underscore? they're called from external code, > so aren't really private. Fixed. > ::: browser/base/content/newtab/drag.js > > +/** > > + * This singleton implements site dragging functionality. > > should this singleton work at the level of cells, not sites? not sure why > this code needs to be aware of a cell's contents - looking at it, it's all > about the actual container itself. > > drop.js handles cells in the drop code, but sites in the drag code. can this > be made consistent, or is that not possible for some reason? Cells are static and will never move around, be removed or change their positions. Sites float around, get deleted and created. So we're always dragging sites onto cells. This is needed to have all these grid transformation effects and to allow dropping arbitrary links onto the grid. We could have fewer sites than cells and a user could drag a bookmark and drop it onto a cell. We'll then create a new site and put it into the target cell. > > + * Creates a new site in the grid. > > + * @param aLink The new site's link. > > + * @param aCell The new site's parent cell. > > when you say 'parent', you mean it's the cell that the site will be created > in, right? maybe 'containing' would be better than 'parent'? parent connotes > the ability to have multiple children. Fixed. > where does the cell come from? > > what happens as a result of this? is an existing cell pushed off the grid? > should document the side-effects. Cells in the grid are static. So even with not a single thumbnail visible we still have 9 cells created because they're static containers of sites moving around dynamically and being created/deleted. > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > +Cu.import("resource://gre/modules/Services.jsm"); > > +Cu.import("resource://gre/modules/Geometry.jsm"); > > +Cu.import("resource:///modules/PageThumbs.jsm"); > > +Cu.import("resource:///modules/NewTabUtils.jsm"); > > worth lazy-loading these? I used defineLazyModuleGetter() for Geometry.jsm as that's only used when transforming the grid. All other JSMs will be needed when creating the grid (i.e. loading the page), so they're not worth lazy-loading. > > + * Checks if the page is modified and sets the CSS class accordingly > > + */ > > + checkIfModified: function Page_checkIfModified() { > > the method name doesn't indicate any side-effects. this doesn't just check, > it sets a value as well. maybe updateModifiedFlag or something ungainly like > that? Changed to updateModifiedFlag(), I think that's expressive. > > + doc.addEventListener("dragover", this._onDragOver, false); > > + doc.addEventListener("drop", this._onDrop, false); > > these aren't really private, so shouldn't be underscore prefixed. Fixed. > ::: browser/base/content/newtab/transformations.js > > + * This singleton allows to transform the grid by moving sites around. > > should say whether it works on sites, cells, site-nodes or cell-nodes, or a > mix of the above. Fixed. > ::: browser/base/content/newtab/updater.js > > + _findRemainingSites: function Updater_findRemainingSites(aLinks) { > > can you clarify the name and comment to say what these are remaining from? > what's the datatype of aLinks? > what's the datatype of the return value? Done. > > + _moveSiteNodes: function Updater_moveSiteNodes(aSites) { > > + let cells = gGrid.cells; > > + let sites = aSites.slice(0, cells.length); > > imagine i'm a dev looking at this code for the first time, trying to fix a > bug or add a feature... why does this need to happen? can you add some more > commentary to explain the context of this call? Done.
Attachment #585454 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #586278 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 141•12 years ago
|
||
Disabling the New Tab Page by default because that's how we're gonna land it.
Attachment #584434 -
Attachment is obsolete: true
Attachment #586988 -
Flags: review?(gavin.sharp)
Attachment #584434 -
Flags: review?(gavin.sharp)
Comment 142•12 years ago
|
||
Comment on attachment 585398 [details] [diff] [review] Part 2 - Assets / CSS / Images >+++ b/browser/base/content/newtab/newTab.css >+#vbox:not([page-disabled]) { Please find a better id for this. >+.button, >+.button::-moz-focus-inner { >+ cursor: pointer; >+} What's the point of ::-moz-focus-inner here? >+++ b/browser/themes/gnomestripe/newtab/newTab.css >+#body { >+ font-family: -moz-window, sans-serif; When would the sans-serif fallback be needed? >+.button, >+.button::-moz-focus-inner { >+ padding: 0; >+ border: 0 none; >+} Is there still some kind of focus indicator?
Assignee | ||
Comment 143•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #142) > >+#vbox:not([page-disabled]) { > > Please find a better id for this. Sorry, that was a bad choice. Changed to #scrollbox because that's what it's for. > >+.button, > >+.button::-moz-focus-inner { > >+ cursor: pointer; > >+} > > What's the point of ::-moz-focus-inner here? Um, that's legacy from before I moved theme specific rules to separate files. Removed. > >+#body { > >+ font-family: -moz-window, sans-serif; > > When would the sans-serif fallback be needed? Also legacy from the very beginning when we used a custom font that wasn't available on all machines. Removed. > >+.button, > >+.button::-moz-focus-inner { > >+ padding: 0; > >+ border: 0 none; > >+} > > Is there still some kind of focus indicator? That's the actual place where we want to remove the 'inner outline'.
Attachment #585398 -
Attachment is obsolete: true
Attachment #587682 -
Flags: review?(dao)
Attachment #585398 -
Flags: review?(dao)
Comment 144•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #143) > > >+.button, > > >+.button::-moz-focus-inner { > > >+ padding: 0; > > >+ border: 0 none; > > >+} > > > > Is there still some kind of focus indicator? > > That's the actual place where we want to remove the 'inner outline'. This doesn't seem to answer my question. Are these buttons keyboard-focusable? Is there a focus indicator?
Assignee | ||
Comment 145•12 years ago
|
||
Yes, they are. The focus indicator is also visible after clicking those buttons. I guess we shouldn't remove the focus indicator because of accessibility? I don't oppose having it at all but it's pretty ugly when using only the mouse to click those buttons.
Comment 146•12 years ago
|
||
Yes, keyboard focus requires an indicator. :-moz-focusring should let you target keyboard focus vs. mouse focus.
Assignee | ||
Comment 147•12 years ago
|
||
So I had a look at bug 418521 and seems like this behavior is default. I mainly developed on Linux where the focus indicators are always shown and I don't liked that. But if that's the default we should probably don't touch it? Also, I won't even be able to detect if the element has been focused via keyboard (and not mouse) on Linux... What do you think?
Comment 148•12 years ago
|
||
Comment on attachment 586278 [details] [diff] [review] Part 4 - Shared Module >diff --git a/browser/modules/NewTabUtils.jsm b/browser/modules/NewTabUtils.jsm >+function PrivateBrowsingStorage(aStorage) { >+ this._data = {}; Seems like this should use Dict.jsm. >diff --git a/toolkit/content/Services.jsm b/toolkit/content/Services.jsm >+ ["privateBrowsing", "@mozilla.org/privatebrowsing;1", "nsIPrivateBrowsingService"], This will fail for apps that don't have a private browsing service. You could do a run-time check to decide whether it should be included, but I think it's easier/better to just avoid adding this.
Comment 149•12 years ago
|
||
Comment on attachment 586988 [details] [diff] [review] Part 3 - about:newtab integration Perhaps we should use "blank page URL" rather than "new tab URL", in the utilityOverlay constant name and helper function? It isn't only used for "new tabs". I guess it doesn't matter too much, but we probably can't rename it later :) >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+ NewTabUtils.init(); Do you really want this to be called for each browser window? It seems like maybe init() should return early if the cache has already been populated. >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >- var blank = !aURI || (aURI == "about:blank"); >+ var blank = !aURI || isNewTabURL(aURI); >- if (!blank) { >+ if (aURI && aURI != "about:blank") { >- // pretend the user typed this so it'll be available till >- // the document successfully loads >- b.userTypedValue = aURI; >+ if (blank) { >+ // The progress listener received a stop status change. Reset >+ // the mBlank status to true to prevent the throbber from being >+ // shown when we're still loading a new tab URL. >+ tabListener.mBlank = true; >+ } else { >+ // pretend the user typed this so it'll be available till >+ // the document successfully loads >+ b.userTypedValue = aURI; >+ } Can you move these changes to a followup ("make sure throbber doesn't appear for about:newtab"?). I want to look into this in more detail, and it doesn't need to block the rest of this landing. You can change the |if (blank)| check to if (blank || aURI && isNewTabURL(aURI)) to fix the tab title setting part.
Attachment #586988 -
Flags: review?(gavin.sharp) → review+
Comment 150•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #147) > So I had a look at bug 418521 and seems like this behavior is default. I > mainly developed on Linux where the focus indicators are always shown and I > don't liked that. But if that's the default we should probably don't touch > it? yep
Assignee | ||
Comment 151•12 years ago
|
||
Removed the ::-moz-focus-inner selectors.
Attachment #587682 -
Attachment is obsolete: true
Attachment #588071 -
Flags: review?(dao)
Attachment #587682 -
Flags: review?(dao)
Comment 152•12 years ago
|
||
Comment on attachment 588071 [details] [diff] [review] Part 2 - Assets / CSS / Images I tested this on Windows. font-family: -moz-window; doesn't seem to work, I'm getting Times New Roman. I can tab through the items, but other than the status panel, I get no indication which item is focused. I also can't seem to active items with Enter. With the new tab page hidden, the empty page has a scroll bar if the window is small enough.
Attachment #588071 -
Flags: review?(dao) → review-
Assignee | ||
Comment 153•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #148) > >diff --git a/browser/modules/NewTabUtils.jsm b/browser/modules/NewTabUtils.jsm > > >+function PrivateBrowsingStorage(aStorage) { > >+ this._data = {}; > > Seems like this should use Dict.jsm. Why exactly would it be better to use this? The API differs from nsIDOMStorage and as I need to clone the DOMStorage data there is also no way to easily initialize a Dict with a plain JavaScript object so I'm not really seeing the benefits here. Am I missing something? > >diff --git a/toolkit/content/Services.jsm b/toolkit/content/Services.jsm > > >+ ["privateBrowsing", "@mozilla.org/privatebrowsing;1", "nsIPrivateBrowsingService"], > > This will fail for apps that don't have a private browsing service. You > could do a run-time check to decide whether it should be included, but I > think it's easier/better to just avoid adding this. Removed this, thanks.
Assignee | ||
Comment 154•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #152) > font-family: -moz-window; doesn't seem to work, I'm getting Times New Roman. No idea why that couldn't be working but I wonder if we should just use 'sans-serif'. That looks much better for me. > I can tab through the items, but other than the status panel, I get no > indication which item is focused. I also can't seem to active items with > Enter. Need to fix this. > With the new tab page hidden, the empty page has a scroll bar if the window > is small enough. Darn, that got lost when renaming #vbox to #scrollbox. I forgot to adapt a selector in the code.
Assignee | ||
Comment 155•12 years ago
|
||
Updating patch.
Attachment #586278 -
Attachment is obsolete: true
Assignee | ||
Comment 156•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #149) > >+ NewTabUtils.init(); > > Do you really want this to be called for each browser window? It seems like > maybe init() should return early if the cache has already been populated. No we don't want that... Fixed, thanks!
Assignee | ||
Comment 157•12 years ago
|
||
Patch updated with the following: * Hiding scrollbars when the grid is hidden. * Using sans-serif as default font. * Sites are now focusable and have an outline when focused. When focused the thumbnails fade to full opacity. Keyboard navigation: It's now possible to use the keyboard to properly navigate the New Tab Page. To update the tab indices for focusable elements changes to Part 2 were necessary and I'll update the patch in a minute. The pin and remove buttons on every site are not focusable for now as they're currently hidden in the black strip at the top. Bug 716532 will address this.
Attachment #588071 -
Attachment is obsolete: true
Attachment #589189 -
Flags: review?(dao)
Assignee | ||
Comment 158•12 years ago
|
||
Sorry, have to ask for review again because of some changes necessary to allow keyboard navigation. Hopefully interdiff can help here. Changes: * When hiding/showing/modifying the grid make sure that the right buttons are focusable (and some are not) and the sites have the right tab indices. * Let the toolbar pass the focus to the next button when the currently focused button is soon to be hidden. * Make pin/remove buttons not focusable for now - this is tackled in bug 716532. * Some other minor fixes might be included, not sure. I'll file a follow-up bug about writing a test for keyboard navigation and 'focus flow' to make sure we don't regress this.
Attachment #586378 -
Attachment is obsolete: true
Attachment #589193 -
Flags: review?(dietrich)
Assignee | ||
Comment 159•12 years ago
|
||
Fixed a typo and renamed "updateTabIndeces" to "updateTabIndices".
Attachment #589193 -
Attachment is obsolete: true
Attachment #589197 -
Flags: review?(dietrich)
Attachment #589193 -
Flags: review?(dietrich)
Comment 160•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #153) > Why exactly would it be better to use this? The API differs from > nsIDOMStorage and as I need to clone the DOMStorage data there is also no > way to easily initialize a Dict with a plain JavaScript object so I'm not > really seeing the benefits here. Am I missing something? I meant that PrivateBrowsingStorage's _data member should be a Dict, not that you should replace PrivateBrowsingStorage itself with a Dict. You can initialize it using set() in the loop, just as you're initializing the current object. We should generally always use Dict.jsm rather than plain objects when you want a simple key-data store, to avoid problems with e.g. |'toSource' in {}|.
Comment 161•12 years ago
|
||
latest Part 1 interdiffs: https://bugzilla.mozilla.org/attachment.cgi?oldid=586378&action=interdiff&newid=589193&headers=1 https://bugzilla.mozilla.org/attachment.cgi?oldid=589193&action=interdiff&newid=589197&headers=1
Comment 162•12 years ago
|
||
Comment on attachment 589197 [details] [diff] [review] Part 1 - XHTML Page / Scripts Review of attachment 589197 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/toolbar.js @@ +66,5 @@ > + > + /** > + * Passes the focus from the current button to the next. > + * @param aCurrent The button that currently has focus. > + * @param aNext The button that is focused next. please document that these are parts of the element ids.
Attachment #589197 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 163•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #162) > please document that these are parts of the element ids. Done.
Assignee | ||
Comment 164•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #160) > I meant that PrivateBrowsingStorage's _data member should be a Dict, not > that you should replace PrivateBrowsingStorage itself with a Dict. You can > initialize it using set() in the loop, just as you're initializing the > current object. We should generally always use Dict.jsm rather than plain > objects when you want a simple key-data store, to avoid problems with e.g. > |'toSource' in {}|. Ok, gotcha, done.
Assignee | ||
Comment 165•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #149) > Perhaps we should use "blank page URL" rather than "new tab URL", in the > utilityOverlay constant name and helper function? It isn't only used for > "new tabs". I guess it doesn't matter too much, but we probably can't rename > it later :) I totally concur that it's definitely better to rename isNewTabURL() to isBlankPageURL() because that's what we're basically testing. But IMHO we shouldn't rename BROWSER_NEW_TAB_URL to BROWSER_BLANK_PAGE_URL because that gives a false impression. This "const" is a wrapper for the 'browser.newtab.url' pref which basically defines the URL used for new tabs and not every blank page (because that's still about:blank in most cases).
Assignee | ||
Comment 166•12 years ago
|
||
Dão: it would be great if you have time to review the last patch today or tomorrow. We'd like to land the New Tab Page (pref'ed off) this week to give it some Nightly bake time before the Aurora uplift. Thanks!
Updated•12 years ago
|
Attachment #589189 -
Flags: review?(dao) → review+
Assignee | ||
Comment 167•12 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #149) > >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > > >- var blank = !aURI || (aURI == "about:blank"); > >+ var blank = !aURI || isNewTabURL(aURI); > > >- if (!blank) { > >+ if (aURI && aURI != "about:blank") { > > >- // pretend the user typed this so it'll be available till > >- // the document successfully loads > >- b.userTypedValue = aURI; > >+ if (blank) { > >+ // The progress listener received a stop status change. Reset > >+ // the mBlank status to true to prevent the throbber from being > >+ // shown when we're still loading a new tab URL. > >+ tabListener.mBlank = true; > >+ } else { > >+ // pretend the user typed this so it'll be available till > >+ // the document successfully loads > >+ b.userTypedValue = aURI; > >+ } > > Can you move these changes to a followup ("make sure throbber doesn't appear > for about:newtab"?). I want to look into this in more detail, and it doesn't > need to block the rest of this landing. Ok, re-opened bug 716108 and will attach a patch.
Assignee | ||
Comment 168•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/27a45008fc12 https://hg.mozilla.org/integration/fx-team/rev/ce8a25a34c2a https://hg.mozilla.org/integration/fx-team/rev/6fd9a7eb3b01 https://hg.mozilla.org/integration/fx-team/rev/34e078df2ed6 https://hg.mozilla.org/integration/fx-team/rev/c389d719d4ec
Whiteboard: [a11y-ping=davidb] → [a11y-ping=davidb][fixed-in-fx-team]
Target Milestone: Future → Firefox 12
Assignee | ||
Comment 169•12 years ago
|
||
Backed out because bug 497543 screwed up talos. https://hg.mozilla.org/integration/fx-team/rev/0cbcaaf2d512 https://hg.mozilla.org/integration/fx-team/rev/816b9e460e28 https://hg.mozilla.org/integration/fx-team/rev/6fd01ecaaa37 https://hg.mozilla.org/integration/fx-team/rev/e3a59e6affd3 https://hg.mozilla.org/integration/fx-team/rev/84c998ef1689
Whiteboard: [a11y-ping=davidb][fixed-in-fx-team] → [a11y-ping=davidb]
Assignee | ||
Comment 170•12 years ago
|
||
Second try: https://hg.mozilla.org/integration/fx-team/rev/34157f4059ba https://hg.mozilla.org/integration/fx-team/rev/3f30da5d0bc3 https://hg.mozilla.org/integration/fx-team/rev/746adaa9c9da https://hg.mozilla.org/integration/fx-team/rev/95143a881557 https://hg.mozilla.org/integration/fx-team/rev/38eda0c8b0fd
Whiteboard: [a11y-ping=davidb] → [a11y-ping=davidb][fixed-in-fx-team]
Assignee | ||
Comment 171•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/34157f4059ba https://hg.mozilla.org/mozilla-central/rev/3f30da5d0bc3 https://hg.mozilla.org/mozilla-central/rev/746adaa9c9da https://hg.mozilla.org/mozilla-central/rev/95143a881557 https://hg.mozilla.org/mozilla-central/rev/38eda0c8b0fd
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [a11y-ping=davidb][fixed-in-fx-team] → [a11y-ping=davidb]
Comment 172•12 years ago
|
||
w00t \o/
Assignee | ||
Comment 173•12 years ago
|
||
FTR, this feature has landed but is currently disabled by default. If you'd like to help testing and want to use it *now* so please do the following: 0) get a nightly build 1) set browser.newtab.url to 'about:newtab' 2) restart the browser 3) open a new tab and show the grid by clicking the button in the upper right Alternatively you could try the UX branch, it's enabled by default there.
Assignee | ||
Comment 174•12 years ago
|
||
Please do NOT report any issues you notice here. Please file new bugs. Thanks!
Assignee | ||
Comment 175•12 years ago
|
||
Backed out because this bug is heavily regressing Ts/Tshutdown times and we don't know why, yet. Sorry for the hassle :( https://hg.mozilla.org/mozilla-central/rev/3677a84a568b https://hg.mozilla.org/mozilla-central/rev/330a6839466e https://hg.mozilla.org/mozilla-central/rev/02512dac94c9 https://hg.mozilla.org/mozilla-central/rev/8d1334baf2c1 https://hg.mozilla.org/mozilla-central/rev/d1cdc35292f1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 177•12 years ago
|
||
Pushed again together with bug 721413: https://hg.mozilla.org/integration/fx-team/rev/34e387bd3d81 https://hg.mozilla.org/integration/fx-team/rev/f64d87ed4ce7 https://hg.mozilla.org/integration/fx-team/rev/9de64b482a23 https://hg.mozilla.org/integration/fx-team/rev/d394252ee2a8 https://hg.mozilla.org/integration/fx-team/rev/d4b00a9e87b5
Whiteboard: [a11y-ping=davidb] → [a11y-ping=davidb][fixed-in-fx-team]
Assignee | ||
Comment 178•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/34e387bd3d81 https://hg.mozilla.org/mozilla-central/rev/f64d87ed4ce7 https://hg.mozilla.org/mozilla-central/rev/9de64b482a23 https://hg.mozilla.org/mozilla-central/rev/d394252ee2a8 https://hg.mozilla.org/mozilla-central/rev/d4b00a9e87b5
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [a11y-ping=davidb][fixed-in-fx-team] → [a11y-ping=davidb]
Comment 179•12 years ago
|
||
Is there a bug filed for undo-ing removal of a site/thumbnail from the new tab page?
Comment 180•12 years ago
|
||
you can click the refresh button at top right of the new tab page
Comment 181•12 years ago
|
||
(In reply to bogas04 from comment #180) > you can click the refresh button at top right of the new tab page What if I want to restore the last removed site. There is a similar option in Google Chrome. Also the refresh icon doesn't convey the idea of reset.
Assignee | ||
Comment 182•12 years ago
|
||
(In reply to Siddhartha Dugar (sdrocking) from comment #181) > What if I want to restore the last removed site. There is a similar option > in Google Chrome. Please go ahead and file a bug. > Also the refresh icon doesn't convey the idea of reset. Feel free and file a bug about that, too. (This is not the right place guys, file new bugs please...)
Comment 183•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #182) > Please go ahead and file a bug. Filed bug 722234 and bug 722235. Please confirm.
Comment 184•12 years ago
|
||
So I expect to find this in the nighties?
Assignee | ||
Comment 185•12 years ago
|
||
Yes, see bug 716538.
Updated•12 years ago
|
Updated•12 years ago
|
Keywords: addon-compat
Assignee | ||
Updated•12 years ago
|
Updated•12 years ago
|
Comment 186•12 years ago
|
||
Bug 740340 asks to revert patch "Part 3" unless it's actually working. As is, the pref doesn't work, and the changes break existing addons that hook up a newtab page.
Depends on: 740340
Updated•12 years ago
|
Comment 187•10 years ago
|
||
FYI: There's a new "New Tab Page" component for bugs.
Component: Tabbed Browser → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•