Closed
Bug 917805
Opened 11 years ago
Closed 11 years ago
Current tab is briefly visible when you open an external link
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 27
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Margaret
:
review+
bnicholson
:
feedback+
|
Details | Diff | Splinter Review |
Often happens when you open an external URL while about:home was visible.
Comment 1•11 years ago
|
||
We used to block for things like this, worth tracking?
tracking-fennec: --- → ?
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → lucasr.at.mozilla
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #807855 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #807856 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #807857 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #807860 -
Flags: review?(margaret.leibovic)
Attachment #807860 -
Flags: feedback?(bnicholson)
Comment 6•11 years ago
|
||
Comment on attachment 807855 [details] [diff] [review]
Factor out method to forcefully clear content are
Review of attachment 807855 [details] [diff] [review]:
-----------------------------------------------------------------
I think it makes more sense to put this function in LayerView.java if you're going to factor it out. It's also easier to call because generally code outside the gfx/ has to go through the LayerView to get the LayerRenderer anyway.
Attachment #807855 -
Flags: review?(bugmail.mozilla) → review+
Comment 7•11 years ago
|
||
Comment on attachment 807860 [details] [diff] [review]
Force UI state when opening an external URL with ACTION_VIEW
Review of attachment 807860 [details] [diff] [review]:
-----------------------------------------------------------------
Don't we already do this when adding a new tab within Fennec? It seems like we should be able to follow a similar code path with loading an external URL. We have tab stubs, so that means we shouldn't be waiting for Gecko to respond for these UI changes. If we are, I think we should try to fix those cases by converting them to tab stubs instead of making special cases.
When we fire the tab selected callback (which I think happens immediately when using stubs), we update the home pager state: http://hg.mozilla.org/mozilla-central/file/d50f590056fd/mobile/android/base/BrowserApp.java#l188. So I'm wondering if you could fix this bug simply by using Tabs.getInstance().loadUrl() directly in ACTION_VIEW instead of messaging Gecko here: http://hg.mozilla.org/mozilla-central/file/d50f590056fd/mobile/android/base/GeckoApp.java#l1827. While here, you could also get rid of the ACTION_LOAD intent since that's not used anymore.
BTW, I had a similar WIP patch in bug 878156 that never got landed (https://bugzilla.mozilla.org/attachment.cgi?id=760013), so maybe I could look into reviving it if it's useful.
::: mobile/android/base/BrowserApp.java
@@ +2225,5 @@
> return new ServiceNotificationClient(getApplicationContext());
> }
>
> + /**
> + * Forces the state of our UI to immediatelly react to new a target
s/immediatelly/immediately/
s/new a/a new/
Comment 8•11 years ago
|
||
Comment on attachment 807856 [details] [diff] [review]
Remove unused animateHideHomePager(), simplify hideHomePager()
Review of attachment 807856 [details] [diff] [review]:
-----------------------------------------------------------------
Nice, this will make this logic easier to follow.
Attachment #807856 -
Flags: review?(margaret.leibovic) → review+
Comment 9•11 years ago
|
||
Comment on attachment 807857 [details] [diff] [review]
Add API to forcefully hide HomePager
Review of attachment 807857 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/BrowserApp.java
@@ +1567,5 @@
> return;
> }
>
> final Tab tab = Tabs.getInstance().getSelectedTab();
> + if (!force && tab != null && isAboutHome(tab)) {
Add a comment here to explain the scenario that satisfies these conditions.
Attachment #807857 -
Flags: review?(margaret.leibovic) → review+
Comment 10•11 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #7)
> So I'm wondering if you could fix this bug simply by
> using Tabs.getInstance().loadUrl() directly in ACTION_VIEW instead of
> messaging Gecko here:
> http://hg.mozilla.org/mozilla-central/file/d50f590056fd/mobile/android/base/
> GeckoApp.java#l1827.
This sounds like an interesting idea, especially because it would be nice to better support calling Tabs.getInstance().loadUrl() directly. We currently never do that from user-generated events, but that's what some of our tests do, and I think it would reduce flakiness if we actually supported it.
Updated•11 years ago
|
Status: NEW → ASSIGNED
Updated•11 years ago
|
tracking-fennec: ? → +
Comment 11•11 years ago
|
||
Comment on attachment 807860 [details] [diff] [review]
Force UI state when opening an external URL with ACTION_VIEW
f- until we can try comment 7.
Attachment #807860 -
Flags: feedback?(bnicholson) → feedback-
Assignee | ||
Comment 12•11 years ago
|
||
Using Tabs.loadUrl() did the trick. None of the patches are necessary anymore. I'll only keep the one that removes animateHideHomePager because it's a worthy code cleanup anyway.
Attachment #807855 -
Attachment is obsolete: true
Attachment #807857 -
Attachment is obsolete: true
Attachment #807860 -
Attachment is obsolete: true
Attachment #807860 -
Flags: review?(margaret.leibovic)
Attachment #812125 -
Flags: review?(margaret.leibovic)
Attachment #812125 -
Flags: feedback?(bnicholson)
Comment 13•11 years ago
|
||
Comment on attachment 812125 [details] [diff] [review]
Use Tabs.loadUrl() for immediate reaction to ACTION_VIEW intents
Cool.
Attachment #812125 -
Flags: feedback?(bnicholson) → feedback+
Updated•11 years ago
|
Attachment #812125 -
Attachment is patch: true
Comment 14•11 years ago
|
||
Comment on attachment 812125 [details] [diff] [review]
Use Tabs.loadUrl() for immediate reaction to ACTION_VIEW intents
Review of attachment 812125 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
Attachment #812125 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/961a0bc211ea
https://hg.mozilla.org/mozilla-central/rev/94709aa52803
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
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
•