Closed
Bug 723251
Opened 14 years ago
Closed 8 years ago
onSaveInstanceState() never saves screenshots to instance state bundle
Categories
(Firefox for Android Graveyard :: General, defect, P5)
Tracking
(blocking-fennec1.0 -)
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
blocking-fennec1.0 | --- | - |
People
(Reporter: cpeterson, Unassigned)
References
Details
(Whiteboard: [splashscreen])
Attachments
(2 files, 18 obsolete files)
19.77 KB,
patch
|
Details | Diff | Splinter Review | |
20.05 KB,
patch
|
Details | Diff | Splinter Review |
GeckoApp.onSaveInstanceState() never saves screenshots to the bundle. onSaveInstanceState() nulls out the most recent screenshot (mLastScreen), calls an asynchronous function (SessionSnapshotRunnable) to refresh the screenshot, and then immediately saves the null mLastScreen. SessionSnapshotRunnable fires a Gecko "Tab:Screenshot" event, which is asynchronous.
Updated•14 years ago
|
Assignee: nobody → jdhaliwal
Comment 1•14 years ago
|
||
this patch also contains fixes for screenshot viewing via placeholder layer client
Updated•14 years ago
|
Attachment #593995 -
Flags: review?(bnicholson)
Comment 2•14 years ago
|
||
minor corrections to previous patch. sorry!
Attachment #594031 -
Flags: review?(bnicholson)
Comment 3•14 years ago
|
||
Comment on attachment 594031 [details] [diff] [review]
jdhaliwalPatch01-2
Review of attachment 594031 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoApp.java
@@ -553,5 @@
> if (outState == null)
> outState = new Bundle();
>
> - new SessionSnapshotRunnable(null).run();
> -
You only take screenshots now on document stop. What if the page has changed since then?
@@ -606,5 @@
> processThumbnail(tab, null, thumbnail);
> return;
> }
>
> - mLastScreen = null;
If we don't set mLastScreen to null before taking the screenshot, and we don't finish the screenshot before being killed, there's a good chance the screenshot will be stale (e.g., a screenshot from another tab). No screenshot is better than a screenshot that doesn't match the page.
@@ +644,5 @@
> Log.w(LOGTAG, "decoding byte array ran out of memory", ome);
> }
> +
> + final Bitmap bmpCopy = BitmapFactory.decodeByteArray(compressed, 0, compressed.length);
> +
We do decodeByteArray() right above this in the try/catch - can you refactor so that only gets called once?
@@ +646,5 @@
> +
> + final Bitmap bmpCopy = BitmapFactory.decodeByteArray(compressed, 0, compressed.length);
> +
> + if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
> + mMainHandler.post(new Runnable() {
Why is this on the main thread?
@@ +651,5 @@
> + public void run() {
> + if (bmpCopy != null) {
> + ByteArrayOutputStream bos = new ByteArrayOutputStream();
> + bmpCopy.compress(Bitmap.CompressFormat.PNG, 0, bos);
> + mLastScreen = bos.toByteArray();
It looks like you are decoding the compressed byte array, compressing it again, and getting another byte array. Can you just use the original byte array, "compressed"?
@@ +1284,5 @@
> mBrowserToolbar.setProgressVisibility(false);
> onTabsChanged(tab);
> +
> + if (mLastScreen != null)
> + connectGeckoLayerClient();
Why connect the layer client on every document stop?
@@ +1688,5 @@
> String uri = getURIFromIntent(intent);
> if (uri != null && uri.length() > 0)
> passedUri = mLastTitle = uri;
>
> + if (mLastScreen == null && (passedUri == null || passedUri.equals("about:home"))) {
Why check mLastScreen == null here?
Attachment #594031 -
Flags: review?(bnicholson) → review-
Comment 4•14 years ago
|
||
CC'ing blassey since he did a lot of the screenshot code.
Reporter | ||
Comment 5•14 years ago
|
||
Comment on attachment 594031 [details] [diff] [review]
jdhaliwalPatch01-2
Review of attachment 594031 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoApp.java
@@ -553,5 @@
> if (outState == null)
> outState = new Bundle();
>
> - new SessionSnapshotRunnable(null).run();
> -
To expand on bnicholson's comment, if the current layer type allows synchronous screenshot captures, we should grab one here. And in that case, do we still need to take screenshots of all pages in handleDocumentStop()?
@@ +645,5 @@
> }
> +
> + final Bitmap bmpCopy = BitmapFactory.decodeByteArray(compressed, 0, compressed.length);
> +
> + if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
You should check isSelectedTab() before decodeByteArray() to avoid unnecessary decoding.
@@ +648,5 @@
> +
> + if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
> + mMainHandler.post(new Runnable() {
> + public void run() {
> + if (bmpCopy != null) {
You should check bmpCopy != null before mMainHandler.post() to avoid unnecessary handlers.
::: xpcom/tests/unit/data/presentation.key/Contents/PkgInfo
@@ -1,1 @@
> -????????
\ No newline at end of file
Why is PkgInfo modified?
Comment 6•14 years ago
|
||
cleaned up code, added separation of synchronous/asynchronous screenshots, corrected minor issues
Attachment #593995 -
Attachment is obsolete: true
Attachment #594031 -
Attachment is obsolete: true
Attachment #593995 -
Flags: review?(bnicholson)
Attachment #594354 -
Flags: review?(bnicholson)
Comment 7•14 years ago
|
||
Also, in regards to cpeterson's mention about the capability of synchronous screenshots on saving the instance's bundle, capturing screenshots synchronously using getBitmap() isn't functional AFAIK. I did add logic so that if it does fail and return null from getBitmap(), then it would revert to using the asynchronous method, but for a full screen size instead of a thumbnail size. However, this also nulls out the previously saved full size screenshot, which can affect other things dealing with session restore mostly.
Comment 8•14 years ago
|
||
Hopefully this is the final version of this patch. Fixed some memory errors in the most recent version, and added some extra concurrency to decoding of byte arrays (which I suggest we do more when possible to allow GeckoAppShell thread more freedom to run and render the content on the page, instead of doing useless background stuff)
Attachment #594354 -
Attachment is obsolete: true
Attachment #594354 -
Flags: review?(bnicholson)
Attachment #594785 -
Flags: review?(bnicholson)
Comment 9•14 years ago
|
||
Comment on attachment 594785 [details] [diff] [review]
jdhaliwalPatch01-4
Review of attachment 594785 [details] [diff] [review]:
-----------------------------------------------------------------
If possible, we should remove mLastScreen. Rather than saving a screenshot on every document stop and holding it in mLastScreen, we should only take the screenshot in onPause().
::: mobile/android/base/GeckoApp.java
@@ -564,5 @@
> public class SessionSnapshotRunnable implements Runnable {
> - Tab mThumbnailTab;
> - SessionSnapshotRunnable(Tab thumbnailTab) {
> - mThumbnailTab = thumbnailTab;
> - }
We should pass in the Tab here, and use that instead of getSelectedTab(), in case the document stop happens on a non-selected tab.
@@ +637,4 @@
> }
> }
>
> + void processThumbnail(final Tab thumbnailTab, final byte[] compressed) {
getSelectedTabScreenshot() converts the bitmap to a byte array and gives it to processThumbnail, which then converts it back to a bitmap. You may want to overload processThumbnail:
void processThumbnail(final Tab thumbnailTab, final byte[] compressed) {...}
void processThumbnail(final Tab thumbnailTab, final Bitmap bitmap) {...}
The byte[] version can decompress the byte array to a bitmap, then call the 2nd version.
@@ +1707,1 @@
> // show about:home if we aren't restoring previous session
Remove the | mLastScreen == null | check
Attachment #594785 -
Flags: review?(bnicholson) → review-
Comment 10•14 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #9)
> @@ +1707,1 @@
> > // show about:home if we aren't restoring previous session
>
> Remove the | mLastScreen == null | check
Oops, I was referring to this line:
+ if (mLastScreen == null && (passedUri == null || passedUri.equals("about:home"))) {
Comment 11•14 years ago
|
||
Did some cleaning in regards to getting rid of unnecessary variables being kept in memory, and added some extra concurrency to operations involving bitmaps
Attachment #594785 -
Attachment is obsolete: true
Attachment #595109 -
Flags: review?(bnicholson)
Comment 12•14 years ago
|
||
Removed threadding adjustments for a separate patch, and made other minor changes
Attachment #595109 -
Attachment is obsolete: true
Attachment #595109 -
Flags: review?(bnicholson)
Attachment #595209 -
Flags: review?(bnicholson)
Comment 13•14 years ago
|
||
Comment on attachment 595209 [details] [diff] [review]
jdhaliwalPatch01-6
Review of attachment 595209 [details] [diff] [review]:
-----------------------------------------------------------------
You should be able to get rid of mLastScreen entirely. Save the last screen as a local variable in GeckoApp.onCreate(), then pass it to the PlaceholderLayerClient.
Other comments/nits:
::: mobile/android/base/GeckoApp.java
@@ +389,5 @@
> return true;
> }
>
> @Override
> + public boolean onPrepareOptionsMenu(final Menu aMenu)
remove this final
@@ +409,5 @@
> public void run() {
> try {
> URL url = new URL(item.icon);
> InputStream is = (InputStream) url.getContent();
> + final Drawable drawable = Drawable.createFromStream(is, "src");
remove this final
@@ +1271,5 @@
> mBrowserToolbar.setProgressVisibility(false);
> onTabsChanged(tab);
> +
> + if (mLastScreen != null)
> + connectGeckoLayerClient();
move this back into the Gecko:Ready listener
@@ +1751,5 @@
> *
> * TODO: Fall back to a built-in screenshot of the Fennec Start page for a nice first-
> * run experience, perhaps?
> */
> +
remove empty line
Comment 14•14 years ago
|
||
Summary of our conversation: when connectGeckoLayerClient() was in Gecko:Ready, it was connected too early. That is, the screenshot came up briefly, but this flashed to white and loaded the page. Ideally, the layer client will be switched after the page is loaded to minimize any jarring transitions. You addressed this by moving connectGeckoLayerClient() to document stop instead of Gecko:Ready.
It'd still be nice to get rid of mLastScreen, but you're currently using it in document stop to determine whether or not to switch out the layer client. I think you can just use "mPlaceHolderLayerClient != null" as a substitute.
The other issue is that if we change pages while still showing the screenshot (e.g., we change the URL of the current tab or we switch tabs), the screenshot will stay there until the next page has loaded. You can try fixing this by adding the last URL to the bundle, then passing that URL to the layer client when we restore the bundle (no need for a global mLastUrl). You can then do something like this in loadRequest():
if (mPlaceHolderLayerClient != null && !mPlaceHolderLayerClient.getPlaceHolderUrl().equals(url)) {
// destroy placeholderlayerclient and connect geckolayerclient
}
Comment 15•14 years ago
|
||
got rid of mLastScreen and applied changes to rely on PlaceHolderLayerClient to determine if the last session's screenshot is being displayed, and for checking the last session's url for determining when to switch to the GeckoSoftwareLayerClient.
Attachment #595209 -
Attachment is obsolete: true
Attachment #595209 -
Flags: review?(bnicholson)
Attachment #595467 -
Flags: review?(bnicholson)
Comment 16•14 years ago
|
||
Changed so if a synchronous screenshot cannot be taken on exit of the app, then an asynchronous screenshot will be taken. Now a screenshot is almost always saved.
Attachment #595467 -
Attachment is obsolete: true
Attachment #595467 -
Flags: review?(bnicholson)
Attachment #595579 -
Flags: review?(bnicholson)
Comment 17•14 years ago
|
||
Comment on attachment 595579 [details] [diff] [review]
jdhaliwalPatch01-8
Originally, one of the goals here was to remove mLastScreen. The two problems with it are 1) we have to take the screenshot in Gecko for every document stop, which is expensive, and 2) we need to hold it in memory. The screenshot is only used when resuming from a saved state to initialize the placeholder client, so theoretically, we should only have to take the screenshot when we are saving the state (and we would then immediately save its byte array into the bundle).
Unfortunately, gitBitmap() isn't reliable, so we need to take screenshots on the Gecko side if we want them to work. If we end up keeping screenshots, we should really have a synchronous Java screenshot method that we call when saving the instance state. But for now, this patch is still an improvement over what we had since screenshots previously didn't work at all.
Attachment #595579 -
Flags: review?(bnicholson) → review+
Comment 18•13 years ago
|
||
Comment on attachment 595579 [details] [diff] [review]
jdhaliwalPatch01-8
># HG changeset patch
># Parent 766a59650976905670d77135b218df8f6762c843
>added code to not show screenshot on session restore from external link
>
>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>--- a/mobile/android/base/GeckoApp.java
>+++ b/mobile/android/base/GeckoApp.java
>@@ -137,24 +137,24 @@ abstract public class GeckoApp
> public static BrowserToolbar mBrowserToolbar;
> public static DoorHangerPopup mDoorHangerPopup;
> public static AutoCompletePopup mAutoCompletePopup;
> public Favicons mFavicons;
>
> private Geocoder mGeocoder;
> private Address mLastGeoAddress;
> private static LayerController mLayerController;
>- private static PlaceholderLayerClient mPlaceholderLayerClient;
>+ private static PlaceholderLayerClient mPlaceholderLayerClient = null;
> private static GeckoSoftwareLayerClient mSoftwareLayerClient;
> private AboutHomeContent mAboutHomeContent;
> private static AbsoluteLayout mPluginContainer;
>
>- public String mLastTitle;
>- public String mLastViewport;
>- public byte[] mLastScreen;
>+ public String mLastTitle = null;
>+ public String mLastViewport = null;
>+ public byte[] mLastScreen = null;
> public int mOwnActivityDepth = 0;
> private boolean mRestoreSession = false;
>
> public interface OnTabsChangedListener {
> public void onTabsChanged(Tab tab);
> }
>
> private static ArrayList<OnTabsChangedListener> mTabsChangedListeners;
>@@ -385,17 +385,17 @@ abstract public class GeckoApp
> {
> sMenu = menu;
> MenuInflater inflater = getMenuInflater();
> inflater.inflate(R.layout.gecko_menu, menu);
> return true;
> }
>
> @Override
>- public boolean onPrepareOptionsMenu(Menu aMenu)
>+ public boolean onPrepareOptionsMenu(final Menu aMenu)
> {
> Iterator<ExtraMenuItem> i = sExtraMenuItems.iterator();
> while (i.hasNext()) {
> final ExtraMenuItem item = i.next();
> if (aMenu.findItem(item.id) == null) {
> final MenuItem mi = aMenu.add(Menu.NONE, item.id, Menu.NONE, item.label);
> if (item.icon != null) {
> if (item.icon.startsWith("data")) {
>@@ -405,17 +405,17 @@ abstract public class GeckoApp
> mi.setIcon(drawable);
> }
> else if (item.icon.startsWith("jar:") || item.icon.startsWith("file://")) {
> GeckoAppShell.getHandler().post(new Runnable() {
> public void run() {
> try {
> URL url = new URL(item.icon);
> InputStream is = (InputStream) url.getContent();
>- Drawable drawable = Drawable.createFromStream(is, "src");
>+ final Drawable drawable = Drawable.createFromStream(is, "src");
> mi.setIcon(drawable);
> } catch (Exception e) {
> Log.w(LOGTAG, "onPrepareOptionsMenu: Unable to set icon", e);
> }
> }
> });
> }
> }
>@@ -548,107 +548,102 @@ abstract public class GeckoApp
> protected void onSaveInstanceState(Bundle outState) {
> super.onSaveInstanceState(outState);
> if (mOwnActivityDepth > 0)
> return; // we're showing one of our own activities and likely won't get paged out
>
> if (outState == null)
> outState = new Bundle();
>
>- new SessionSnapshotRunnable(null).run();
>-
>- outState.putString(SAVED_STATE_TITLE, mLastTitle);
>- outState.putString(SAVED_STATE_VIEWPORT, mLastViewport);
>- outState.putByteArray(SAVED_STATE_SCREEN, mLastScreen);
>- outState.putBoolean(SAVED_STATE_SESSION, true);
>+ getSessionSnapshot(outState);
> }
>
>- public class SessionSnapshotRunnable implements Runnable {
>- Tab mThumbnailTab;
>- SessionSnapshotRunnable(Tab thumbnailTab) {
>- mThumbnailTab = thumbnailTab;
>- }
>-
>- public void run() {
>- synchronized (mSoftwareLayerClient) {
>- Tab tab = Tabs.getInstance().getSelectedTab();
>- if (tab == null)
>- return;
>-
>- HistoryEntry lastHistoryEntry = tab.getLastHistoryEntry();
>- if (lastHistoryEntry == null)
>- return;
>-
>- if (getLayerController().getLayerClient() != mSoftwareLayerClient)
>- return;
>-
>- ViewportMetrics viewportMetrics = mSoftwareLayerClient.getGeckoViewportMetrics();
>- if (viewportMetrics != null)
>- mLastViewport = viewportMetrics.toJSON();
>-
>- mLastTitle = lastHistoryEntry.mTitle;
>- getAndProcessThumbnailForTab(tab, true);
>- }
>- }
>- }
>-
>- void getAndProcessThumbnailForTab(final Tab tab, boolean forceBigSceenshot) {
>- boolean isSelectedTab = Tabs.getInstance().isSelectedTab(tab);
>- final Bitmap bitmap = isSelectedTab ?
>- mSoftwareLayerClient.getBitmap() : null;
>-
>+ public void getSessionSnapshot(final Bundle outState) {
>+ /* This function saves the state of the current browsing session, which includes:
>+ * - a screenshot of currently selected tab
>+ * - the title of the last history entry for the currently selected tab
>+ * - a boolean to indicate a saved state
>+ */
>+
>+ final Tab tab = Tabs.getInstance().getSelectedTab();
>+ if (tab == null)
>+ return;
>+
>+ final Bitmap bitmap = mSoftwareLayerClient.getBitmap();
>+
> if (bitmap != null) {
> ByteArrayOutputStream bos = new ByteArrayOutputStream();
> bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);
>- processThumbnail(tab, bitmap, bos.toByteArray());
>+ bitmap.recycle();
>+ outState.putByteArray(SAVED_STATE_SCREEN, bos.toByteArray());
> } else {
>- if (!tab.hasLoaded()) {
>+ // This is to handle the case where getBitmap() would return null (eg. for now, phones using gralloc)
>+ outState.putByteArray(SAVED_STATE_SCREEN, mLastScreen);
>+ }
>+
>+ HistoryEntry lastHistoryEntry = tab.getLastHistoryEntry();
>+ if (lastHistoryEntry != null) {
>+ outState.putString(SAVED_STATE_TITLE, lastHistoryEntry.mTitle);
>+ outState.putString(SAVED_STATE_URI, lastHistoryEntry.mUri);
>+ }
>+
>+ if (getLayerController().getLayerClient() == mSoftwareLayerClient) {
>+ ViewportMetrics viewportMetrics = mSoftwareLayerClient.getGeckoViewportMetrics();
>+ if (viewportMetrics != null)
>+ outState.putString(SAVED_STATE_VIEWPORT, viewportMetrics.toJSON());
>+ }
>+
>+ outState.putBoolean(SAVED_STATE_SESSION, true);
>+ }
>+
>+ void getAsyncThumbnailForTab(final Tab tab, boolean forceBigScreenshot) {
>+ // This function captures and processes a screenshot asynchronously for any tab, and can specify the screenshot to be thumbnail sized or not
>+ if (!tab.hasLoaded()) {
>+ if (!forceBigScreenshot) {
> byte[] thumbnail = BrowserDB.getThumbnailForUrl(getContentResolver(), tab.getURL());
>- if (thumbnail != null)
>- processThumbnail(tab, null, thumbnail);
>- return;
>+ if (thumbnail != null) {
>+ processThumbnail(tab, thumbnail);
>+ }
> }
>-
>- mLastScreen = null;
>- int sw = forceBigSceenshot ? mSoftwareLayerClient.getWidth() : tab.getMinScreenshotWidth();
>- int sh = forceBigSceenshot ? mSoftwareLayerClient.getHeight(): tab.getMinScreenshotHeight();
>- int dw = forceBigSceenshot ? sw : tab.getThumbnailWidth();
>- int dh = forceBigSceenshot ? sh : tab.getThumbnailHeight();
>- try {
>- JSONObject message = new JSONObject();
>- message.put("tabID", tab.getId());
>-
>- JSONObject source = new JSONObject();
>- source.put("width", sw);
>- source.put("height", sh);
>- message.put("source", source);
>-
>- JSONObject destination = new JSONObject();
>- source.put("width", dw);
>- source.put("height", dh);
>- message.put("destination", destination);
>-
>- String json = message.toString();
>- GeckoAppShell.sendEventToGecko(new GeckoEvent("Tab:Screenshot", json));
>- } catch(JSONException jsonEx) {
>- Log.w(LOGTAG, "Constructing the JSON data for Tab:Screenshot event failed", jsonEx);
>- }
>+ return;
>+ }
>+
>+ int sw = forceBigScreenshot ? mSoftwareLayerClient.getWidth() : tab.getMinScreenshotWidth();
>+ int sh = forceBigScreenshot ? mSoftwareLayerClient.getHeight(): tab.getMinScreenshotHeight();
>+ int dw = forceBigScreenshot ? sw : tab.getThumbnailWidth();
>+ int dh = forceBigScreenshot ? sh : tab.getThumbnailHeight();
>+ try {
>+ JSONObject message = new JSONObject();
>+ message.put("tabID", tab.getId());
>+
>+ JSONObject source = new JSONObject();
>+ source.put("width", sw);
>+ source.put("height", sh);
>+ message.put("source", source);
>+
>+ JSONObject destination = new JSONObject();
>+ destination.put("width", dw);
>+ destination.put("height", dh);
>+ message.put("destination", destination);
>+
>+ String json = message.toString();
>+ GeckoAppShell.sendEventToGecko(new GeckoEvent("Tab:Screenshot", json));
>+ } catch(JSONException jsonEx) {
>+ Log.w(LOGTAG, "Constructing the JSON data for Tab:Screenshot event failed", jsonEx);
> }
> }
>
>- void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
>- if (Tabs.getInstance().isSelectedTab(thumbnailTab))
>- mLastScreen = compressed;
>+ void processThumbnail(final Tab thumbnailTab, final byte[] compressed) {
>+ //NOTE: compressed is assumed to be an encoded byte array (most likely PNG format)
> if (thumbnailTab.getURL().equals("about:home")) {
> thumbnailTab.updateThumbnail(null);
> return;
> }
> try {
>- if (bitmap == null)
>- bitmap = BitmapFactory.decodeByteArray(compressed, 0, compressed.length);
>+ Bitmap bitmap = BitmapFactory.decodeByteArray(compressed, 0, compressed.length);
> thumbnailTab.updateThumbnail(bitmap);
> } catch (OutOfMemoryError ome) {
> Log.w(LOGTAG, "decoding byte array ran out of memory", ome);
> }
> }
>
> private void maybeCancelFaviconLoad(Tab tab) {
> long faviconLoadId = tab.getFaviconLoadId();
>@@ -983,17 +978,16 @@ abstract public class GeckoApp
> mMainHandler.post(new Runnable() {
> public void run() {
> if (sMenu != null)
> sMenu.findItem(R.id.settings).setEnabled(true);
> }
> });
> setLaunchState(GeckoApp.LaunchState.GeckoRunning);
> GeckoAppShell.sendPendingEventsToGecko();
>- connectGeckoLayerClient();
> } else if (event.equals("ToggleChrome:Hide")) {
> mMainHandler.post(new Runnable() {
> public void run() {
> mBrowserToolbar.setVisibility(View.GONE);
> }
> });
> } else if (event.equals("ToggleChrome:Show")) {
> mMainHandler.post(new Runnable() {
>@@ -1090,16 +1084,27 @@ abstract public class GeckoApp
> tab.setHasTouchListeners(true);
> if (Tabs.getInstance().isSelectedTab(tab)) {
> mMainHandler.post(new Runnable() {
> public void run() {
> mLayerController.setWaitForTouchListeners(true);
> }
> });
> }
>+ } else if (event.equals("Tab:ScreenshotData")) {
>+ final Tab tab = Tabs.getInstance().getTab(message.getInt("tabID"));
>+ final String data = message.getString("data");
>+ if (data.length() < 22)
>+ return;
>+ final byte[] compressed = GeckoAppShell.decodeBase64(data.substring(22));
>+ if (Tabs.getInstance().isSelectedTab(tab) && message.getInt("height") == mSoftwareLayerClient.getHeight() && message.getInt("width") == mSoftwareLayerClient.getWidth()) {
>+ mLastScreen = compressed;
>+ } else {
>+ processThumbnail(tab, compressed);
>+ }
> }
> } catch (Exception e) {
> Log.e(LOGTAG, "Exception handling message \"" + event + "\":", e);
> }
> }
>
> public void showAboutHome() {
> Runnable r = new AboutHomeRunnable(true);
>@@ -1266,26 +1271,28 @@ abstract public class GeckoApp
>
> void handleDocumentStop(int tabId) {
>+ getAsyncThumbnailForTab(tab,true);
We don't want to do this for any tab except the selected tab
> mMainHandler.post(new Runnable() {
>+
>+ if (mPlaceholderLayerClient != null)
>+ connectGeckoLayerClient();
What's this doing? Deferring setting up GL until we have a page?
>+ GeckoAppShell.registerGeckoEventListener("Tab:ScreenshotData", GeckoApp.mAppContext);
Do we need to move this back into GeckoApp from Tab?
Attachment #595579 -
Flags: review-
Comment 19•13 years ago
|
||
Comment on attachment 595579 [details] [diff] [review]
jdhaliwalPatch01-8
>+ if (mPlaceholderLayerClient != null && !(mPlaceholderLayerClient.getPlaceHolderUri().equals(url))) {
>+ mPlaceholderLayerClient.destroy();
>+ connectGeckoLayerClient();
>+ }
Why do we need this code in addition to the handleDocumentStop code?
Comment 20•13 years ago
|
||
Hey Mark, just to reply to your comments:
1) The getAsyncThumbnailForTab(tab,true) call in handleDocumentStop is there so that on document stop we get a full size screenshot for the selected tab. In getAsyncThumbnail there is logic which immediately returns if we try to get a full size screenshot for any other tab other than the currently selected one.
2) That call to connectGeckoLayerClient is there so that when the PlaceholderLayerClient is present, and the page if fully loaded, then we switch out to the GeckoSoftwareLayerClient to prevent a jarring transition (which would happen if we did it earlier)
3) The Tab:ScreenshotData event was moved to GeckoApp.java for cleanup purposes only. I was mostly following advice given by bnicholson, and I agree with him that this event didn't necessarily need to be in Tab.java, since we deal with the rest of the screenshot stuff here.
4) The other call to connectGeckoLayerClient is made so that if the user tries to load another page (by switching a tab or entering in the address bar), then we immediately connect the GeckoSoftwareLayerClient so that the screenshot doesn't remain on the screen while the user is trying to load/view another page.
Comment 21•13 years ago
|
||
updated to include newer non-canvas screenshot/thumbnail code from bug 724210 & bug 726930
Note: Tab:ScreenshotData event no longer exists with the changes involving the newer non-canvas screenshot/thumbnail code
Attachment #595579 -
Attachment is obsolete: true
Attachment #599795 -
Flags: review?(cpeterson)
Attachment #599795 -
Flags: review?(bnicholson)
Reporter | ||
Comment 22•13 years ago
|
||
Comment on attachment 599795 [details] [diff] [review]
jdhaliwalPatch01-9
Review of attachment 599795 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoApp.java
@@ +149,5 @@
>
> + public String mLastTitle = null;
> + public String mLastSnapshotUri = null;
> + public String mLastViewport = null;
> + public byte[] mLastScreen = null;
Fields will default to null, so you don't need to initialize them to null.
@@ +562,3 @@
> }
>
> + public void getSessionSnapshot(final Bundle outState) {
Can getSessionSnapshot() be private? It only seems to be called from GeckoApp.
@@ +565,5 @@
> + /* This function saves the state of the current browsing session, which includes:
> + * - a screenshot of currently selected tab
> + * - the title of the last history entry for the currently selected tab
> + * - a boolean to indicate a saved state
> + */
Function doc comments should be outside the function.
@@ +573,5 @@
> + return;
> +
> + final Bitmap bitmap = mSoftwareLayerClient.getBitmap();
> +
> + if (bitmap != null) {
On which devices does getBitmap() return non-null here? Do we need to support both code paths?
@@ +635,5 @@
>
> void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
> + if (Tabs.getInstance().isSelectedTab(thumbnailTab)
> + && bitmap.getWidth() == mSoftwareLayerClient.getWidth()
> + && bitmap.getHeight() == mSoftwareLayerClient.getHeight()) {
When does bitmap not match mSoftwareLayerClient's dimensions? Would that be a bug or a valid error condition?
@@ +2617,5 @@
> } else {
> GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Tab:Load", args.toString()));
> }
> + if (mPlaceholderLayerClient != null && !(mLastSnapshotUri.equals(url))) {
> + connectGeckoLayerClient();
Can you add a comment describing the situation when connectGeckoLayerClient() will be called from here instead of from handleDocumentStop()?
Updated•13 years ago
|
Keywords: fennecnative-releaseblocker
Priority: -- → P2
Comment 23•13 years ago
|
||
In response to cpeterson,
1) I'm just a bit paranoid, so initializing to null is just to be extra safe, since I check their values somewhat often ;). I don't really feel that it disrupts anything too much either.
2) Ah, good catch. I'll switch that to private.
3) I felt like having it inside the function is a bit clearer for being able to tell which function the comment is for, but if we have an enforced standard for that, then I can change it :)
4) I'm not really sure if we're keeping getBitmap() or not, but I don't really think anyone would object to it disappearing, and they can always choose an older version if they really want it. Consider it gone.
5) processThumbnail is also used to generate thumbnails for the tabs tray, so we could be calling it with a small thumbnail-sized bitmap. This is to prevent unnecessarily setting mLastScreen.
6) Yup, I'll add that in.
Attachment #599795 -
Attachment is obsolete: true
Attachment #599795 -
Flags: review?(cpeterson)
Attachment #599795 -
Flags: review?(bnicholson)
Attachment #600104 -
Flags: review?(cpeterson)
Attachment #600104 -
Flags: review?(bnicholson)
Reporter | ||
Comment 24•13 years ago
|
||
> 5) processThumbnail is also used to generate thumbnails for the tabs tray, so we could be
> calling it with a small thumbnail-sized bitmap. This is to prevent unnecessarily setting
> mLastScreen.
This suggests that processThumbnail() is doing two jobs in one function and should be split into two methods, something like processThumbnail() and processScreenshot(). But that refactoring is probably beyond the scope of this patch.
Comment 25•13 years ago
|
||
Needed to change handleDocumentStop behaviour. There is a race condition between a resize event triggered from connecting the GeckoSoftwareLayerClient, and the screenshot event from the call to getAsyncThumbnailForTab. For now, I added a delay to the screenshot event, but this should be moved to a separate event if the session restore screenshot becomes a confirmed feature.
Attachment #600104 -
Attachment is obsolete: true
Attachment #600104 -
Flags: review?(cpeterson)
Attachment #600104 -
Flags: review?(bnicholson)
Attachment #600446 -
Flags: review?(cpeterson)
Attachment #600446 -
Flags: review?(bnicholson)
Updated•13 years ago
|
blocking-fennec1.0: --- → +
Reporter | ||
Comment 26•13 years ago
|
||
Comment on attachment 600446 [details] [diff] [review]
jdhaliwalPatch01-11
Review of attachment 600446 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoApp.java
@@ +1326,5 @@
> + GeckoAppShell.getHandler().postDelayed(new Runnable() {
> + public void run() {
> + getAsyncThumbnailForTab(tab,true);
> + }
> + }, 2000);
If you move this call to getAsyncThumbnailForTab() outside the Runnable's `if (mPlaceholderLayerClient != null)` check, can you then remove the getAsyncThumbnailForTab() block before the Runnable?
getAsyncThumbnailForTab() is called from many places, so reducing the number of calls will make this code more consistent and easier to understand.
Updated•13 years ago
|
Status: NEW → ASSIGNED
Comment 27•13 years ago
|
||
@cpeterson:
The two calls to getAsyncThumbnailForTab() are to handle different cases. I'm assuming that we want to start the async screenshot event as soon as possible, so for a tab that's already loaded and ready to have a screenshot taken, there's no point in waiting for the runnable (being run on the main UI thread) to complete, so that's why we have a call before the creation of the runnable. However, the other call inside the runnable is to handle the case where we need to wait until after the GSLC connects and resizes before taking a screenshot.
We could move it so we only have one call to getAsyncThumbnailForTab() (it would need to be inside the runnable, since that's connecting the GSLC), but we would (in most cases) be unnecessarily delaying the screenshot event. I was writing this with the intention to have the screenshots happen as early as possible after the page is loaded and ready to have a screenshot taken, so that's the reason why there are two control paths. Let me know what you think.
Reporter | ||
Comment 28•13 years ago
|
||
@bnicholson, do you have an opinion on getAsyncThumbnailForTab()?
I have an opinion, but not a strong one. If you two prefer two calls to getAsyncThumbnailForTab(), that's OK with me.
I feel consolidating the two code paths (thus reducing special cases and ensuring the one code path is well-tested) is more useful than taking a screenshot ASAP.
In fact, delaying the screenshot seems like a good idea on its own. Taking a screenshot can use a lot of memory, so delaying the screenshot could give the system some time to "catch its breath" after loading a web page. Plus, the page layout may shift dynamically after loading. And if the user taps a link in less than 2 seconds, we don't take an unnecessary screenshot. :)
Comment 29•13 years ago
|
||
cpeterson's argument sounds good to me. I think we should just have one call to getAsyncThumbnailForTab().
Comment 30•13 years ago
|
||
Changed from two code paths for taking a screenshot to one consolidated path. This happens after the GeckoSoftwareLayerClient is connected (if it needs to be connected) to handle both cases that were described earlier. It should work for most websites.
I'll also attach another patch soon which allows the screenshot to be written to file instead of in a bundle, since the bundle can allow for inconsistencies.
Attachment #600446 -
Attachment is obsolete: true
Attachment #600446 -
Flags: review?(cpeterson)
Attachment #600446 -
Flags: review?(bnicholson)
Attachment #603159 -
Flags: review?(cpeterson)
Attachment #603159 -
Flags: review?(bnicholson)
Comment 31•13 years ago
|
||
Attachment #603160 -
Flags: review?(cpeterson)
Attachment #603160 -
Flags: review?(bnicholson)
Reporter | ||
Comment 32•13 years ago
|
||
Comment on attachment 603160 [details] [diff] [review]
writeToFile Patch
Review of attachment 603160 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoApp.java
@@ +116,2 @@
> public static final String SAVED_STATE_SESSION = "session";
> + public static final String SAVED_STATE_FILESIZE = "filesize";
All these SAVED_STATE_* fields should be private. No other classes use these fields.
@@ +151,5 @@
> public String mLastTitle = null;
> public String mLastSnapshotUri = null;
> public String mLastViewport = null;
> public byte[] mLastScreen = null;
> + public int mLastScreenFilesize;
mLastScreenFileSize should be private. Non-static, non-final fields should only have public (or package) visibility in extenuating circumstances.
@@ +574,5 @@
> + outState.putInt(SAVED_STATE_FILESIZE, mLastScreen.length);
> + if (mLastScreen != null) {
> + try {
> + File outfile = new File(SAVED_STATE_SCREEN);
> + FileOutputStream fos = new FileOutputStream(outfile);
You don't need `outfile` if you use FileOutputStream(String path) constructor with a filename.
@@ +577,5 @@
> + File outfile = new File(SAVED_STATE_SCREEN);
> + FileOutputStream fos = new FileOutputStream(outfile);
> + fos.write(mLastScreen);
> + fos.close();
> + } catch (Exception e) {
You should catch the most specific exception. In this case, FileOutputStream's constructor, write(), and close() all throw IOException.
@@ +579,5 @@
> + fos.write(mLastScreen);
> + fos.close();
> + } catch (Exception e) {
> + Log.e(LOGTAG, "Failed to write last screenshot to file!");
> + e.printStackTrace();
Only log exception stack traces for bugs we should fix, not normal error conditions.
In this case a file exception is a normal error condition (no sdcard), not a bug that would require fixing, so we can log a message but we should not log the exception stack trace.
Also, if you want to report an exception indicating a bug, you should use Log.e(LOGTAG, "DESCRIPTION", e).
@@ +1712,5 @@
> + byte[] lastScreen = null;
> +
> + if (mLastScreenFilesize > 0) {
> + try {
> + File infile = new File(SAVED_STATE_SCREEN);
You don't need `infile` if you use the FileInputStream(String path) constructor with a filename (and catch FileNotFoundException or IOException).
@@ +1716,5 @@
> + File infile = new File(SAVED_STATE_SCREEN);
> + if (infile.exists()) {
> + FileInputStream fis = new FileInputStream(infile);
> + lastScreen = new byte[mLastScreenFilesize];
> + fis.read(lastScreen,0,mLastScreenFilesize);
Insert spaces after commas.
@@ +1719,5 @@
> + lastScreen = new byte[mLastScreenFilesize];
> + fis.read(lastScreen,0,mLastScreenFilesize);
> + fis.close();
> + }
> + } catch (Exception e) {
Catch IOException, not all Exceptions.
@@ +1721,5 @@
> + fis.close();
> + }
> + } catch (Exception e) {
> + Log.e(LOGTAG, "Failed to read last screenshot from file!");
> + e.printStackTrace();
Only log exception stack traces for bugs we should fix, not normal error conditions.
::: mobile/android/base/GeckoEvent.java
@@ +236,5 @@
> PointF geckoPoint = new PointF(event.getX(eventIndex), event.getY(eventIndex));
> geckoPoint = GeckoApp.mAppContext.getLayerController().convertViewPointToLayerPoint(geckoPoint);
>
> + if (geckoPoint == null)
> + return;
Is this an unrelated change that "snuck" into your screenshot file patch?
Reporter | ||
Comment 33•13 years ago
|
||
Comment on attachment 603159 [details] [diff] [review]
jdhaliwalPatch01-12
Review of attachment 603159 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoApp.java
@@ +544,5 @@
> public String getLastViewport() {
> return mLastViewport;
> }
>
> +
Remove extra whitespace. There should only be one empty line between method definitions.
@@ +557,5 @@
> + getSessionSnapshot(outState);
> + }
> +
> +
> +
Remove extra whitespace. There should only be one empty line between method definitions.
@@ +580,5 @@
> +
> + if (getLayerController().getLayerClient() == mSoftwareLayerClient) {
> + ViewportMetrics viewportMetrics = mSoftwareLayerClient.getGeckoViewportMetrics();
> + if (viewportMetrics != null)
> + outState.putString(SAVED_STATE_VIEWPORT, viewportMetrics.toJSON());
Add braces even for single-line conditional statements.
@@ +608,5 @@
> // If the title, uri and viewport haven't changed, the old screenshot is probably valid
> if (viewportJSON.equals(mLastViewport) &&
> mLastTitle.equals(lastHistoryEntry.mTitle) &&
> mLastSnapshotUri.equals(lastHistoryEntry.mUri))
> + return;
Could mLastTitle or mLastSnapshotUri be null here if we did not restore state from a bundle?
@@ +1291,5 @@
> mBrowserToolbar.setProgressVisibility(false);
> onTabsChanged(tab);
> +
> + if (mPlaceholderLayerClient != null)
> + connectGeckoLayerClient();
Add braces even for single-line conditional statements.
@@ +2610,5 @@
> GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Tab:Load", args.toString()));
> }
> +
> + // If placeholder snapshot screenshot is being shown and we're trying to load a request (switching tab, loading new url, etc.)
> + // that is different from the snapshot's url, then switch to showing the GeckoSoftwareLayerClient to load this new url
Wrap lines at 100 columns.
@@ +2611,5 @@
> }
> +
> + // If placeholder snapshot screenshot is being shown and we're trying to load a request (switching tab, loading new url, etc.)
> + // that is different from the snapshot's url, then switch to showing the GeckoSoftwareLayerClient to load this new url
> + if (mPlaceholderLayerClient != null && !(mLastSnapshotUri.equals(url))) {
* Remove unnecessary parens around mLastSnapshotUri.equals().
* Could mLastSnapshotUri be null here if we did not restore state from a bundle?
::: mobile/android/base/Tabs.java
@@ +145,5 @@
> });
>
> // Pass a message to Gecko to update tab state in BrowserApp
> GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Tab:Selected", String.valueOf(tab.getId())));
> + GeckoApp.mAppContext.getAsyncThumbnailForTab(tab,true);
Insert space after comma.
@@ +307,5 @@
> while (iterator.hasNext()) {
> final Tab tab = iterator.next();
> GeckoAppShell.getHandler().post(new Runnable() {
> public void run() {
> + GeckoApp.mAppContext.getAsyncThumbnailForTab(tab,false);
Insert space after comma.
Comment 34•13 years ago
|
||
This should resolve most of the issues you talked about cpeterson.
About whether mLastTitle/mLastSnapshotUri could be null if we dont restore from a bundle, I'm not sure if/when we would not restore from a bundle, but until I can guarantee that it never happens (since I think it does in some rare cases like OOM) I put in checks just in case.
Attachment #603159 -
Attachment is obsolete: true
Attachment #603160 -
Attachment is obsolete: true
Attachment #603159 -
Flags: review?(cpeterson)
Attachment #603159 -
Flags: review?(bnicholson)
Attachment #603160 -
Flags: review?(cpeterson)
Attachment #603160 -
Flags: review?(bnicholson)
Attachment #603792 -
Flags: review?(cpeterson)
Attachment #603792 -
Flags: review?(bnicholson)
Comment 35•13 years ago
|
||
Attachment #603793 -
Flags: review?(cpeterson)
Attachment #603793 -
Flags: review?(bnicholson)
Reporter | ||
Comment 36•13 years ago
|
||
Comment on attachment 603793 [details] [diff] [review]
writeToFile Patch
Review of attachment 603793 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoApp.java
@@ +1712,5 @@
> + if (mLastScreenFilesize > 0) {
> + try {
> + File infile = new File(SAVED_STATE_SCREEN);
> + if (infile.exists()) {
> + FileInputStream fis = new FileInputStream(infile);
Replace `new File` and `infile.exists()` with `FileInputStream fis = new FileInputStream(SAVED_STATE_SCREEN)` (which will throw an IOException if the file is not found).
Reporter | ||
Comment 37•13 years ago
|
||
Comment on attachment 603792 [details] [diff] [review]
jdhaliwalPatch01-13
Review of attachment 603792 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me!
Attachment #603792 -
Flags: review?(cpeterson) → review+
Comment 38•13 years ago
|
||
Woops, missed that one ><. Thanks!
Attachment #603793 -
Attachment is obsolete: true
Attachment #603793 -
Flags: review?(cpeterson)
Attachment #603793 -
Flags: review?(bnicholson)
Attachment #603902 -
Flags: review?(cpeterson)
Attachment #603902 -
Flags: review?(bnicholson)
Reporter | ||
Comment 39•13 years ago
|
||
Comment on attachment 603902 [details] [diff] [review]
writeToFile Patch
LGTM
Attachment #603902 -
Flags: review?(cpeterson) → review+
Updated•13 years ago
|
Attachment #603792 -
Flags: review?(bnicholson) → review+
Updated•13 years ago
|
Attachment #603902 -
Flags: review?(bnicholson) → review+
Updated•13 years ago
|
Keywords: checkin-needed
Comment 40•13 years ago
|
||
These patches fail to apply cleanly to mozilla-inbound. Please rebase. Also, to make life easier for those checking in on your behalf, please follow the directions below for your patches. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
Comment 41•13 years ago
|
||
rebased and combined both patches to be applied (since they would be applied successively anyways)
Attachment #603792 -
Attachment is obsolete: true
Attachment #603902 -
Attachment is obsolete: true
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Whiteboard: [splashscreen]
Updated•13 years ago
|
Assignee: jdhaliwal → gbrown
Comment 42•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a5cc8bb70a
Once again, please follow the directions in comment 40 to make it easier for people to check patches in for you. Also, this patch was bitrotted (again). You may want to confirm that I didn't mess anything up when fixing it up for checkin.
Keywords: checkin-needed
Target Milestone: --- → Firefox 14
Updated•13 years ago
|
Assignee: gbrown → jdhaliwal
Comment 43•13 years ago
|
||
I guess my un-bitrotting wasn't right. Backed out for bustage.
https://tbpl.mozilla.org/php/getParsedLog.php?id=10072497&tree=Mozilla-Inbound&full=1#error0
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd09e7a515b7
![]() |
||
Comment 44•13 years ago
|
||
Josh - re-re-assign to me if you don't have time for this.
Comment 46•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 47•13 years ago
|
||
ehr, this is still backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 14 → ---
![]() |
||
Comment 48•13 years ago
|
||
This is Josh's patch, updated significantly for bitrot. It builds and runs, but causes fennec to not display any content! (I don't know why.)
Comment 49•13 years ago
|
||
Comment on attachment 607428 [details] [diff] [review]
patch updated for bitrot -- NOT ready for check-in
if (lastScreen != null) {
mLastScreen = lastScreen;
mPlaceholderLayerClient = new PlaceholderLayerClient(mLayerController, mLastViewport);
mGeckoLayout.addView(mLayerController.getView(), 0);
} else {
mLayerController.setLayerClient(mLayerClient);
}
This is incorrect, and is causing the issue with the content not showing. The addView call is what actually attaches the content to the view. I'm not sure what got changed, but I remember writing this to be something like the following:
if (lastScreen != null) {
mLastScreen = lastScreen;
mPlaceholderLayerClient = new PlaceholderLayerClient(mLayerController, mLastViewport);
mLayerController.setLayerClient(mPlaceholderLayerClient);
} else {
mLayerController.setLayerClient(mGeckoSoftwareLayerClient);
}
mGeckoLayout.addView(mLayerController.getView(), 0);
One case is where we have a screenshot image available, so we create the PlaceholderLayerClient with this image, and set it as the layer client. The other case is where no screenshot is available, so the default GeckoSoftwareLayerClient object is used. The layer client should first be set, then the addView call is made afterwards.
*NOTE:
I'm not sure if the name of the default GeckoSoftwareLayerClient object was changed, but I remember it being named mSoftwareLayerClient, so double check to be sure it's actually a GeckoSoftwareLayerClient object
Comment 50•13 years ago
|
||
Hopefully that helps :)
![]() |
||
Comment 51•13 years ago
|
||
Josh - thanks much for the info...I quite appreciate the explanation.
Unfortunately, I could not use your code fragment:
- PlaceHolderLayerClient is not a LayerClient, so cannot be passed to setLayerClient
- calling setLayerClient at all during initialize() seems to cause crashes
The current PlaceHolderLayerClient checks mLastScreen and respects mLastScreen==null, so actually this all works with fewer changes.
This patch basically works for me except that when restoring a screenshot, the image is distorted -- particularly shrunk vertically. (The png file appears to be correct.) I will double-check the dimensions on restore.
Attachment #607428 -
Attachment is obsolete: true
Updated•13 years ago
|
Assignee: gbrown → nobody
blocking-fennec1.0: + → -
Priority: P2 → P5
![]() |
||
Comment 52•13 years ago
|
||
I couldn't complete this, but it almost works. Now we're disabling the feature - bug 740146.
Attachment #607834 -
Attachment is obsolete: true
Comment 53•8 years ago
|
||
Tab screenshots have not only been disabled, but completely removed from the codebase as well.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 8 years ago
Resolution: --- → WONTFIX
Assignee | ||
Updated•5 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
•