Closed
Bug 895816
Opened 11 years ago
Closed 11 years ago
Tapping "New tab" should immediately create a new tab
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: lucasr, Assigned: Margaret)
References
Details
(Whiteboard: abouthome-hackathon [fixed-fig])
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gbrown
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
...instead of simply entering editing mode. This means that even if you dismiss editing mode, you'd still be in the newly created tab (as opposed to just canceling editing mode).
Reporter | ||
Updated•11 years ago
|
Whiteboard: abouthome-hackathon
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → margaret.leibovic
Assignee | ||
Comment 2•11 years ago
|
||
Well, that was quite simple. If you look below, we basically already do the same thing for adding a new private tab.
Attachment #779875 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 3•11 years ago
|
||
Since we're only ever entering editing mode for the currently open tab, we can get rid of this EditingTarget logic.
Attachment #779878 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 779875 [details] [diff] [review]
(Part 1) Tapping "New tab" should immediately create a new tab
Review of attachment 779875 [details] [diff] [review]:
-----------------------------------------------------------------
Nice and simple. I assume adding tabs from tabs tray and menu both call this very same method?
Attachment #779875 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 779875 [details] [diff] [review]
> (Part 1) Tapping "New tab" should immediately create a new tab
>
> Review of attachment 779875 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Nice and simple. I assume adding tabs from tabs tray and menu both call this
> very same method?
Yes, correct.
Reporter | ||
Comment 6•11 years ago
|
||
Comment on attachment 779878 [details] [diff] [review]
(Part 2) Remove EditingTarget because it's no longer needed
Review of attachment 779878 [details] [diff] [review]:
-----------------------------------------------------------------
Nice,
::: mobile/android/base/BrowserApp.java
@@ +1294,5 @@
> tab.setFaviconLoadId(Favicons.NOT_LOADING);
> }
>
> + public boolean enterEditingMode() {
> + return enterEditingMode(null, HomePager.Page.BOOKMARKS);
Add an enterEditingMode(HomePager.Page page) and use it here? Just so that these calls look cleaner.
@@ +1303,4 @@
> }
>
> + public boolean enterEditingMode(HomePager.Page page) {
> + return enterEditingMode(null, page);
Ditto.
Attachment #779878 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Now that opening a new tab just loads about:home, loading a URL in a new tab can just follow the regular loadUrl() logic.
testNewTab passes locally for me, but we can wait on the try results before landing this.
Attachment #779910 -
Flags: review?(gbrown)
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 779878 [details] [diff] [review]
> (Part 2) Remove EditingTarget because it's no longer needed
>
> Review of attachment 779878 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Nice,
>
> ::: mobile/android/base/BrowserApp.java
> @@ +1294,5 @@
> > tab.setFaviconLoadId(Favicons.NOT_LOADING);
> > }
> >
> > + public boolean enterEditingMode() {
> > + return enterEditingMode(null, HomePager.Page.BOOKMARKS);
>
> Add an enterEditingMode(HomePager.Page page) and use it here? Just so that
> these calls look cleaner.
We already have that right down below. Is it worth the extra method call, rather than just calling directly into the method with the main implementation?
> @@ +1303,4 @@
> > }
> >
> > + public boolean enterEditingMode(HomePager.Page page) {
> > + return enterEditingMode(null, page);
>
> Ditto.
Wait... this is the implementation of enterEditingMode(HomePager.Page page), so we can't do that, right?
Updated•11 years ago
|
Attachment #779910 -
Flags: review?(gbrown) → review+
Assignee | ||
Comment 9•11 years ago
|
||
This version does a bit more cleaning up...
onSearchRequested is never used anymore, so we can get rid of that. With that gone, enterEditingMode is only called in onActivate and when the user selects the "Paste" context menu. Talking about this on IRC, lucasr and I decided it made sense for us to just always open the VISITED page in these two cases, so we don't need to pass the page as a parameter anymore.
Attachment #779878 -
Attachment is obsolete: true
Attachment #780064 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 780064 [details] [diff] [review]
(Part 2) Remove EditingTarget because it's no longer needed (v2)
Review of attachment 780064 [details] [diff] [review]:
-----------------------------------------------------------------
I think onSearchRequested is called when you press the hw search button which is only available on older Android devices. I'd probably keep it around. Looks good otherwise.
Attachment #780064 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 11•11 years ago
|
||
This version keeps onSearchRequested, although I found that's not actually working properly, even on fig nightly, so I filed bug 897274 about that.
Attachment #780064 -
Attachment is obsolete: true
Attachment #780073 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Updated•11 years ago
|
Attachment #780073 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/projects/fig/rev/0ffd6875249b
https://hg.mozilla.org/projects/fig/rev/77efbb48885d
https://hg.mozilla.org/projects/fig/rev/a9f98a5f888f
There are some intermittent oranges on the try run, but I feel like we're okay since testNewTab isn't failing at all (and there's been at least one fully green rc1 and rc2).
Whiteboard: abouthome-hackathon → abouthome-hackathon [fixed-fig]
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0ffd6875249b
https://hg.mozilla.org/mozilla-central/rev/77efbb48885d
https://hg.mozilla.org/mozilla-central/rev/a9f98a5f888f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•