Closed Bug 1243558 Opened 8 years ago Closed 8 years ago

Experiment with linking to screenshots on device from bookmarks panel

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: antlam, Assigned: mcomella)

References

Details

Attachments

(10 files)

Screen Shot 2016-01-28 at 8.50.45 AM.png
116.83 KB, image/png
Details
MozReview Request: Bug 1243558 - Safely close Cursor in ScreenshotObserver. r=ahunt
58 bytes, text/x-review-board-request
ahunt
: review+
Details
MozReview Request: Bug 1243558 - Add UrlAnnotations.getScreenshots method. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1243558 - Add UrlAnnotations.insertScreenshot method. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1243558 - Insert metadata into db when the user takes a screenshot. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1243558 - Add Screenshots smart folder & unpolished screenshot views. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1243558 - Add proper layout to bookmark_screenshot_row. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1243558 - Add click handler to screenshots items in bookmarks. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1243558 - Add telemetry to screenshots click handler. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1243558 - Add nightly flags to screenshots in bookmarks feature. r=sebastian
58 bytes, text/x-review-board-request
sebastian
: review+
Details 
Given that we know users screenshot a fair amount in Fennec, we should start to experiment with things that piggy back of this interaction (e.g. bug 1243166).

If we knew the screenshot that they saved was of a bookmarked page, we should try and link to that image from the Bookmarks panel in Fennec. The image would simply load in the image viewer rather than inside Fennec.
To start, we'll need to actually store the URLs that people are screenshotting.

But first of all, we should see if we can store a location for that image when we observe the screenshot, to see if we'll even be able to open it back up.

This is a good little prototype hacking task!
Attaching very early WIP UI/UX for this just so we have something to visualize in the interim.
From mfinkle:

We already can track when someone takes a screenshot. It would be nice to be able to annotate a hidden bookmark with the URL for two reasons:
1. It shows up faster in awesomescreen
2. it allows us to create a "smart" bookmark folder which lists shared/screenshot pages
I discovered screenshot tracking is not consistent, based on the device – I filed bug 1250355.
Depends on: 1250355
Just to write this down somewhere:
* The MediaStore.Images.ImageColumns (henceforth MS.I.IC) field DATA refers to the path on disk to the file
* One can retrieve a contentUri to the image with ContentUris.withAppendedId(table_uri, MS.I.IC._ID)
* This contentUri can be used to load the bitmap directly: MS.I.Media.getBitmap(ContentResolver, uri)
  - Or a thumbnail: MS.I.T.getThumbnail
The naive solution: we store file uris to the screenshots in the
bookmarks table. This means when the user clicks on a screenshot,
it will open in the browser. Issues:
* The screenshots go directly into the user's bookmarks, rather
than into a separate screenshots folder (there is currently no method
to modify folders, however)
* The user can't share the screenshot directly from the browser and
needs to open it in another app to do so. Ideally, we'd fire an
Intent to load the screenshot in a separate app to start.
* The bookmarks are not a direct representation of the current state
of the media directory - e.g. if a screenshot is removed, the
bookmark entry will not be removed.
* These items will get synced to the other devices bookmarks, but
the links will not work on other platforms
* I think the bookmarks list uses favicons for the image and it
seems non-trivial to set the favicon to a thumbnail

Ideally:
* We'd link the screenshots to the sites they were taken on
* We'd link the screenshots to full-page screenshots
* (maybe) Snackbar has "show" action which links to screenshots in bookmarks

Review commit: https://reviewboard.mozilla.org/r/36105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36105/
Assignee: nobody → michael.l.comella
(In reply to Michael Comella (:mcomella) from comment #7)
> Created attachment 8722586 [details]
> MozReview Request: Bug 1243558 - (WIP) Capture screenshots in main bookmarks
> folder.
> 
> The naive solution: we store file uris to the screenshots in the
> bookmarks table. This means when the user clicks on a screenshot,
> it will open in the browser. Issues:
> * The screenshots go directly into the user's bookmarks, rather
> than into a separate screenshots folder (there is currently no method
> to modify folders, however)

We want to make these bookmarks hidden.

> * The user can't share the screenshot directly from the browser and
> needs to open it in another app to do so. Ideally, we'd fire an
> Intent to load the screenshot in a separate app to start.

Agreed. Opening in Fennec seems less than ideal. We'd open the URL of the page in Fennec. The image should probably be opened in Photos (or appropriate).

> * The bookmarks are not a direct representation of the current state
> of the media directory - e.g. if a screenshot is removed, the
> bookmark entry will not be removed.

We could check for that though. If a screenshot is no longer found, we remove the hidden bookmark from the "Saved pages" folder (or whatever we'd call it). We could keep the hidden bookmark which would still "weigh" awesomescreen searches, which is one of our goals. This is also why we want the page URL to be the bookmark. The location of the screenshot should be an annotation.

> * These items will get synced to the other devices bookmarks, but
> the links will not work on other platforms
> * I think the bookmarks list uses favicons for the image and it
> seems non-trivial to set the favicon to a thumbnail

Not a concern if the bookmark itself is hidden. Also, easier to deal with if we make the page URL _the_ bookmark and annotate the screenshot file path.

> 
> Ideally:
> * We'd link the screenshots to the sites they were taken on

Yes.

> * We'd link the screenshots to full-page screenshots

We don't have full page screenshots. Let's try to use what people are already familiar with.

> * (maybe) Snackbar has "show" action which links to screenshots in bookmarks

Ideally, these bookmarks would be hidden and not synced.
I think the easiest way to do this, with fewest edge cases, is _not_ to make these things bookmarks. You'd need to find every place that currently queries for or writes to bookmarks and make sure it does the right thing. There are a lot of places.


A very simple table like this:

   URL      Type       Value       Timestamp


with a type like 'screenshot' (but use a TINYINT) and file paths for values would allow us to trivially make a virtual bookmark folder for screenshots, if we wanted to.

We can also join this table to bookmarks to find out when a real bookmark has an associated screenshot; this is a URL-keyed dictionary, just like the reader cache.

We can later store other kinds of metadata in this table (e.g., PDFs!), and we can sync this easily (particularly if we add a sync status column).
I assume "URL" would be used to lookup a favicon, for display purposes. Could work.

Reiterating the goals:

Track URLs that people screenshotted (or other save signal) so we can:
1. Display them in a smart folder.
2. Add frecency weight for quicker retrieval in awesomescreen.
3. Open the screenshot (or other format) in an appropriate application.
4. These signals will be part of the Activity Stream work, and we'll need to sync them (the signal, not the screenshot or PDF) at some point. Not this bug.

If these are not bookmarks, then we can offer the same style of editing from the smart folder (#1) which should be fine.
Spoke w/ antlam & barbara about their priorities here. To land on Nightly, we need:
* Having screenshots in a smart folder
* Including the timestamp/date in the appearance of the url item
* Associate the url the screenshot was taken on with the screenshot file uri
* Add data probes

Some extras (or follow-ups):
* Decide how we should open a click on the list item (e.g. send intent to gallery to open image or open in FF)
* Associate screenshot w/ full-page screenshot
* Syncing the data
* Screenshots don't get easily removed from the list if they're removed from the gallery – fix that
* Set favicon (i.e. thumbnail) on list items
* In the "Saved screenshot" snackbar:
  - Indicate this content is available offline
  - Add button to "show" the screenshots smart folder

Some additional items that I haven't talked to antlam/barbara about:
* How to disable this functionality? (e.g. invasion of privacy!)
* Does removing an item from this smart folder remove the screenshot from disk or just FF's representation of it?
* Should screenshots show up in awesomebar queries?

See also comment 10 for Finkle's priorities. I'm thinking about how to combine them now.
As far as implementation is concerned, Richard proposed adding a url metadata table for which I filed bug 1250707.
Comment on attachment 8722585 [details]
MozReview Request: Bug 1243558 - Safely close Cursor in ScreenshotObserver. r=ahunt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36103/diff/1-2/
Attachment #8722585 - Attachment description: MozReview Request: Bug 1243558 - Safely Cursor in ScreenshotObserver. r=ahunt → MozReview Request: Bug 1243558 - Safely close Cursor in ScreenshotObserver. r=ahunt
Attachment #8722585 - Flags: review?(ahunt)
Comment on attachment 8722586 [details]
MozReview Request: Bug 1243558 - Add UrlAnnotations.getScreenshots method. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36105/diff/1-2/
Attachment #8722586 - Attachment description: MozReview Request: Bug 1243558 - (WIP) Capture screenshots in main bookmarks folder. → MozReview Request: Bug 1243558 - Add UrlAnnotations.getScreenshots method. r=sebastian
Attachment #8722586 - Flags: review?(s.kaspari)
Modifying the Bookmarks Adapter & wrapped Cursor felt all-over-the place so I'm
concerned the resulting code may be fragile. However, I can't see a better
solution off the top of my head, meaning it's probably not worth the time to
improve.

Review commit: https://reviewboard.mozilla.org/r/37007/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37007/
Attachment #8724341 - Flags: review?(s.kaspari)
Still TODO:
* Create UX similar to antlam's mocks (note: we have to join with the history table to get the title so the first version will probably just use the page url)
  - There should be a ScreenshotItemRow class, similar to TwoLinePageRow
* Add click handlers for screenshot views
Also:
* We should have a mechanism to clear screenshots with clear private data (follow-up?)
* I get duplicate screenshots inserted into the folder – see bug 1250355
(In reply to Michael Comella (:mcomella) from comment #19)
> * I get duplicate screenshots inserted into the folder – see bug 1250355

Maybe we need to do duplicate protection at the DB insertion level.
Comment on attachment 8722586 [details]
MozReview Request: Bug 1243558 - Add UrlAnnotations.getScreenshots method. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36105/diff/2-3/
Comment on attachment 8724339 [details]
MozReview Request: Bug 1243558 - Add UrlAnnotations.insertScreenshot method. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37003/diff/1-2/
Comment on attachment 8724340 [details]
MozReview Request: Bug 1243558 - Insert metadata into db when the user takes a screenshot. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37005/diff/1-2/
Comment on attachment 8724341 [details]
MozReview Request: Bug 1243558 - Add Screenshots smart folder & unpolished screenshot views. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37007/diff/1-2/
Comment on attachment 8722586 [details]
MozReview Request: Bug 1243558 - Add UrlAnnotations.getScreenshots method. r=sebastian

https://reviewboard.mozilla.org/r/36105/#review33595

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:500
(Diff revision 3)
> +        public enum Key {

I added a similar enum in my patch. :) But I'll let you go first and adapt my patch - it's not ready yet anyways.
Attachment #8722586 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724339 [details]
MozReview Request: Bug 1243558 - Add UrlAnnotations.insertScreenshot method. r=sebastian

https://reviewboard.mozilla.org/r/37003/#review33597

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:62
(Diff revision 2)
> +    public enum ScreenshotValueFields {
> +        SCREENSHOT_PATH("path"),
> +        PAGE_TITLE("title");
> +
> +        private String dbValue;
> +        ScreenshotValueFields(final String dbValue) {
> +            this.dbValue = dbValue;
> +        }
> +
> +        public String getDBValue() {
> +            return dbValue;
> +        }
> +    }

It looks like this enum isn't use anywhere?
Attachment #8724339 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724340 [details]
MozReview Request: Bug 1243558 - Insert metadata into db when the user takes a screenshot. r=sebastian

https://reviewboard.mozilla.org/r/37005/#review33599

Just FYI: ScreenshotObserver requires the STORAGE permission. This means a fresh install on Android 6 will only run this code after the user has performed at least one download and accepted the permission. Currently we only ask for the permission for downloads. If this is getting more important then we can think of asking earlier (maybe during firstrun?).
Attachment #8724340 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724341 [details]
MozReview Request: Bug 1243558 - Add Screenshots smart folder & unpolished screenshot views. r=sebastian

https://reviewboard.mozilla.org/r/37007/#review33601

The code contains two TODOs (layout and adapter). Is this something for a follow-up bug?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:835
(Diff revision 2)
> -            // Wrap cursor to add fake desktop bookmarks and reading list folders
> +        // Wrap cursor to add fake desktop bookmarks and reading list folders

NIT: The commend talks about reading list folders but there's nothing about that in the code (anymore)?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:836
(Diff revision 2)
> -            return new SpecialFoldersCursorWrapper(c, addDesktopFolder);
> +        return new SpecialFoldersCursorWrapper(c, addDesktopFolder, addScreenshotsFolder);

Why do we wrap always now? I guess because SpecialFoldersCursorWrapper handles "no special folders" too?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1510
(Diff revision 2)
> +                mIndexOffset += 1;

Super NIT: The existing code above uses ++ and here += 1.
Attachment #8724341 - Flags: review?(s.kaspari)
Comment on attachment 8724341 [details]
MozReview Request: Bug 1243558 - Add Screenshots smart folder & unpolished screenshot views. r=sebastian

https://reviewboard.mozilla.org/r/37007/#review33701

Woops, forgot the r+.
Attachment #8724341 - Flags: review+
https://reviewboard.mozilla.org/r/37003/#review33597

> It looks like this enum isn't use anywhere?

Removed. I accidentally left this in from one of my other development approaches.
https://reviewboard.mozilla.org/r/37007/#review33601

> Why do we wrap always now? I guess because SpecialFoldersCursorWrapper handles "no special folders" too?

Yeah, it should handle `n >= 0` folders and seemed unnecessary to avoid wrapping.

> Super NIT: The existing code above uses ++ and here += 1.

I feel `++` encourages bad behavior so I thought I was making a statement with `+= 1` but you're right to say it's inconsistent – changed back.
Comment on attachment 8724339 [details]
MozReview Request: Bug 1243558 - Add UrlAnnotations.insertScreenshot method. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37003/diff/2-3/
Comment on attachment 8724340 [details]
MozReview Request: Bug 1243558 - Insert metadata into db when the user takes a screenshot. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37005/diff/2-3/
Comment on attachment 8724341 [details]
MozReview Request: Bug 1243558 - Add Screenshots smart folder & unpolished screenshot views. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37007/diff/2-3/
Comment on attachment 8725037 [details]
MozReview Request: Bug 1243558 - Add proper layout to bookmark_screenshot_row. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37287/diff/1-2/
Comment on attachment 8725038 [details]
MozReview Request: Bug 1243558 - Add click handler to screenshots items in bookmarks. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37289/diff/1-2/
Comment on attachment 8725039 [details]
MozReview Request: Bug 1243558 - Add telemetry to screenshots click handler. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37291/diff/1-2/
Comment on attachment 8725040 [details]
MozReview Request: Bug 1243558 - Add nightly flags to screenshots in bookmarks feature. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37293/diff/1-2/
Comment on attachment 8722585 [details]
MozReview Request: Bug 1243558 - Safely close Cursor in ScreenshotObserver. r=ahunt

https://reviewboard.mozilla.org/r/36103/#review34209

::: mobile/android/base/java/org/mozilla/gecko/ScreenshotObserver.java:135
(Diff revision 2)
> +                    if (cursor != null) {

I don't think we need a nullcheck here in this case - but I guess it's better to keep it just in case we do change the code flow in future?

1) If cr.query() throws, then that Exception will propagate up the stack without ever reaching the try block.
2) If cr.query succeeds, and returns null, we return at the null-check in line 111
3) Hence we only reach the finally {} if cr.query() succeeds and returns a non-null cursor
Attachment #8722585 - Flags: review?(ahunt) → review+
(In reply to Andrzej Hunt :ahunt from comment #44)
> but I guess it's
> better to keep it just in case we do change the code flow in future?

Nope.

1. Having a check there is hinting to future programmers that it can (should?) be null. That's wrong. Future-you will have to read this code more carefully… "nope, the cursor can't be null, but why did old me leave this check here? … could I be wrong?".

2. IDEs should hint if you might be dereferencing a null object reference. Don't keep cargo-cult code around to protect against something that tools will detect.

3. If this code changes, it'll probably change that line, too.
Comment on attachment 8725037 [details]
MozReview Request: Bug 1243558 - Add proper layout to bookmark_screenshot_row. r=sebastian

https://reviewboard.mozilla.org/r/37287/#review34429

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkScreenshotRow.java:65
(Diff revision 2)
> +        return sb.toString();

Alternative: String.format("%s - %s", date, time); (with date/time being the formatted string)
Attachment #8725037 - Flags: review?(s.kaspari) → review+
Comment on attachment 8725038 [details]
MozReview Request: Bug 1243558 - Add click handler to screenshots items in bookmarks. r=sebastian

https://reviewboard.mozilla.org/r/37289/#review34431

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:88
(Diff revision 2)
> +        if (adapter.getOpenFolderType() == BookmarksListAdapter.FolderType.SCREENSHOTS) {
> +            final String fileUrl = "file://" + cursor.getString(cursor.getColumnIndex(BrowserContract.UrlAnnotations.VALUE));
> +            getOnUrlOpenListener().onUrlOpen(fileUrl, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));
> +            return;
> +        }

If we are going to open the screenshot in the browser instead of launching a VIEW intent: Can the user still share this image? It looks like share is disabled when I open a local (image) file in the browser.
Attachment #8725038 - Flags: review?(s.kaspari) → review+
Comment on attachment 8725039 [details]
MozReview Request: Bug 1243558 - Add telemetry to screenshots click handler. r=sebastian

https://reviewboard.mozilla.org/r/37291/#review34433
Attachment #8725039 - Flags: review?(s.kaspari) → review+
Comment on attachment 8725040 [details]
MozReview Request: Bug 1243558 - Add nightly flags to screenshots in bookmarks feature. r=sebastian

https://reviewboard.mozilla.org/r/37293/#review34437

::: mobile/android/base/AppConstants.java.in:353
(Diff revision 2)
> +    public static final boolean SCREENSHOTS_IN_BOOKMARKS_ENABLED = AppConstants.NIGHTLY_BUILD;

Oh, this is handy. It should have done this too in the past instead of fiddling with confvars.sh.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:801
(Diff revision 2)
> -            addScreenshotsFolder = true;
> +            addScreenshotsFolder = AppConstants.SCREENSHOTS_IN_BOOKMARKS_ENABLED;

I should have asked this in one of the previous reviews: Do we always add this folder - even if there are no screenshots?
Attachment #8725040 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #47)
> Comment on attachment 8725038 [details]
> MozReview Request: Bug 1243558 - Add click handler to screenshots items in
> bookmarks. r=sebastian
> 
> https://reviewboard.mozilla.org/r/37289/#review34431
> 
> ::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:88
> (Diff revision 2)
> > +        if (adapter.getOpenFolderType() == BookmarksListAdapter.FolderType.SCREENSHOTS) {
> > +            final String fileUrl = "file://" + cursor.getString(cursor.getColumnIndex(BrowserContract.UrlAnnotations.VALUE));
> > +            getOnUrlOpenListener().onUrlOpen(fileUrl, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));
> > +            return;
> > +        }
> 
> If we are going to open the screenshot in the browser instead of launching a
> VIEW intent: Can the user still share this image? It looks like share is
> disabled when I open a local (image) file in the browser.

I haven't been following the UX discussion around this feature closely, but I think this is an open question we should discuss in another bug.

I know Anthony is working on a UX document to spec out how this feature will behave, so let's not block landing the work here, but at the same time we should make sure we have the appropriate follow-ups filed to iron out the UX details.
https://reviewboard.mozilla.org/r/36103/#review34209

> I don't think we need a nullcheck here in this case - but I guess it's better to keep it just in case we do change the code flow in future?
> 
> 1) If cr.query() throws, then that Exception will propagate up the stack without ever reaching the try block.
> 2) If cr.query succeeds, and returns null, we return at the null-check in line 111
> 3) Hence we only reach the finally {} if cr.query() succeeds and returns a non-null cursor

If we return inside a `try` block, the `finally` clause gets called, hence the null check is necessary.
(In reply to Richard Newman [:rnewman] from comment #45)
> (In reply to Andrzej Hunt :ahunt from comment #44)
> > but I guess it's
> > better to keep it just in case we do change the code flow in future?
> 
> Nope.
> 
> 1. Having a check there is hinting to future programmers that it can
> (should?) be null. That's wrong. Future-you will have to read this code more
> carefully… "nope, the cursor can't be null, but why did old me leave this
> check here? … could I be wrong?".

I'm not sure if this response is specifically to my code or ahunt's thought, but if it's the former, `ContentResolver.query` [1] can return null according to the documentation, hence the null checks.

> 2. IDEs should hint if you might be dereferencing a null object reference.
> Don't keep cargo-cult code around to protect against something that tools
> will detect.

fwiw, I don't think IJ or AS do this as of yet – it is caught by Coverity though (if anyone bothers to check it).

[1]: https://developer.android.com/reference/android/content/ContentResolver.html#query%28android.net.Uri,%20java.lang.String[],%20java.lang.String,%20java.lang.String[],%20java.lang.String%29
https://reviewboard.mozilla.org/r/37287/#review34429

> Alternative: String.format("%s - %s", date, time); (with date/time being the formatted string)

Slightly more readable, but slightly more expensive because a String object needs to be created for both date and time, whereas we can avoid the two allocations with the `StringBuffer` methods I used.

I think my code is easily readable, so I'll leave it.
https://reviewboard.mozilla.org/r/37289/#review34431

> If we are going to open the screenshot in the browser instead of launching a VIEW intent: Can the user still share this image? It looks like share is disabled when I open a local (image) file in the browser.

You can open it by opening the screenshot in another app. I have a follow-up bug to handle this properly – bug 1252316.
https://reviewboard.mozilla.org/r/37293/#review34437

> I should have asked this in one of the previous reviews: Do we always add this folder - even if there are no screenshots?

Yeah, I suppose we do. I filed bug 1254229.
Just to put this somewhere, Margaret sent me [1] for some JS code that takes a screenshot of the current page (and could potentially be used to screenshot the full page).

[1]: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1237087&attachment=8727541
https://hg.mozilla.org/integration/fx-team/rev/f859157882d6d3e9a4eaa8ee2e0d8ab3f6d9d5a7
Bug 1243558 - Safely close Cursor in ScreenshotObserver. r=ahunt

https://hg.mozilla.org/integration/fx-team/rev/d267b59bea6f84169cc42fd65ac526e946db5ebe
Bug 1243558 - Add UrlAnnotations.getScreenshots method. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/018a03e29293c786acf32945eeddee0323370610
Bug 1243558 - Add UrlAnnotations.insertScreenshot method. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/c3d13791acd61defbe2c4e1cd2bc6ca296aa0696
Bug 1243558 - Insert metadata into db when the user takes a screenshot. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/fec48d88f2c0f20b8c02deb6e149e06c1911bbcb
Bug 1243558 - Add Screenshots smart folder & unpolished screenshot views. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/a8263e58f7f2fa996c3698e04b3a70070bf283a3
Bug 1243558 - Add proper layout to bookmark_screenshot_row. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/00981f2dfb2c894d17e1a9f9fb9679a46e07e911
Bug 1243558 - Add click handler to screenshots items in bookmarks. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/e445e99bbab67f950d7912dff79b73343ee1e5f3
Bug 1243558 - Add telemetry to screenshots click handler. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/3c1251e453b161a3557e11e09304193a57505c6c
Bug 1243558 - Add nightly flags to screenshots in bookmarks feature. r=sebastian
(In reply to Michael Comella (:mcomella) from comment #52)
> (In reply to Richard Newman [:rnewman] from comment #45)
> > (In reply to Andrzej Hunt :ahunt from comment #44)
> > > but I guess it's
> > > better to keep it just in case we do change the code flow in future?
> > 
> > Nope.
> > 
> > 1. Having a check there is hinting to future programmers that it can
> > (should?) be null. That's wrong. Future-you will have to read this code more
> > carefully… "nope, the cursor can't be null, but why did old me leave this
> > check here? … could I be wrong?".
> 
> I'm not sure if this response is specifically to my code or ahunt's thought,
> but if it's the former, `ContentResolver.query` [1] can return null
> according to the documentation, hence the null checks.

Ooops, I completely forgot that we run the finally even when we return early - so in the current implementation we do need the nullcheck within finally when closing the cursor.

However, in this case, wouldn't it make more sense to do the initial nullcheck (and early return) before the try block? That saves us having to do multiple nullchecks. Since the code is landed maybe that's not worthwhile right now, but I'm wondering what the ideal pattern here would be since we have plenty more similar situations in our codebase.
(In reply to Andrzej Hunt :ahunt from comment #58)
> However, in this case, wouldn't it make more sense to do the initial
> nullcheck (and early return) before the try block? That saves us having to
> do multiple nullchecks. Since the code is landed maybe that's not worthwhile
> right now, but I'm wondering what the ideal pattern here would be since we
> have plenty more similar situations in our codebase.

I use the following pattern:
final Closeable c = returnsCloseable(...);
try {
  // do stuff with c
  // ...
} finally {
  c.close();
}

Because anything following the Closeable assignment could throw, forcing us to close the cursor. Placing the `try` block directly below the assignment makes this clear. Following that principle, if `c` can be null, then we need to do two null checks.

Doing the null check below the assignment, but above the `try` isn't inherently bad, but I feel it's more likely to lead to someone adding code that is after the assignment but not inside the try block. As such, I'd rather increase safety but (briefly) repeat myself.

Ideally, `returnsCloseable` shouldn't return null – throwing is probably a better recovery mechanism.

However, I'm open to changing my opinions if others think it is worthwhile.
Depends on: 1256747
QA Contact: teodora.vermesan
No longer depends on: 1250355
No longer depends on: 1255243
No longer depends on: 1258321
No longer depends on: 1258323
No longer blocks: 1252307
No longer blocks: 1252309
No longer blocks: 1252316
No longer blocks: 1254229
No longer blocks: offline-browsing
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.