Closed Bug 637031 Opened 9 years ago Closed 9 years ago

Add "bookmark link" to context menu

Categories

(Firefox for Android Graveyard :: Bookmarks, enhancement)

enhancement
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Whiteboard: [strings][has patch][needs review])

Attachments

(1 file)

The main reason I want this is because there's no easy way to add bookmarklets in Fennec.
Severity: normal → enhancement
Attached patch patchSplinter Review
We won't want to land this until after Fennec 4.0 is released.  Just posting a development snapshot, and asking for feedback on whether this is a good idea or not.
Attachment #516690 - Flags: ui-review?(madhava)
Whiteboard: [strings] → [strings][has patch]
I noticed the same thing with Fennec, I'll keep this top-of-mind for after 4. It's0
Whiteboard: [strings][has patch] → [strings][has patch][needs UI review]
Yeah - we should have this. We should be smarter about the inclusion of "Save Link" which is often useless when the page links to an html doc (I think it's there to allow saving of PDFs, etc.)
Attachment #516690 - Flags: ui-review?(madhava)
Attachment #516690 - Flags: ui-review+
Attachment #516690 - Flags: review?(21)
(In reply to comment #3)
> Yeah - we should have this. We should be smarter about the inclusion of "Save
> Link" which is often useless when the page links to an html doc (I think it's
> there to allow saving of PDFs, etc.)

We don't know reliably whether a link will return an HTML file or a PDF file until we follow it.  We could guess based on the file extension (e.g., hide the option if the URL ends in ".html") but this might be incorrect some of the time.  Would that be better or worse than showing it all the time?
Status: NEW → ASSIGNED
Whiteboard: [strings][has patch][needs UI review] → [strings][has patch][needs review]
It seems to me that it almost always a not useful option, in a menu that we're trying to keep as short as possible. Most of the time, a link is a link to another page, in which case we just save the HTML which is not a feature I think we would ever prioritize (or even want, particularly). What is a file that a person _would_ want to save, sight unseen (i.e. vs. first seeing an image file and long-tapping it directly to save it)?  PDFs already end up in the download manager.
Filed bug 642996 for discussion of changes to the "Save Link" menu item.
Comment on attachment 516690 [details] [diff] [review]
patch

>diff --git a/chrome/content/ContextCommands.js b/chrome/content/ContextCommands.js
>+    let bookmarks = PlacesUtils.bookmarks;
>+    try {
>+      bookmarks.insertBookmark(BookmarkList.panel.mobileRoot,
>+                               Util.makeURI(state.linkURL),
>+                               bookmarks.DEFAULT_INDEX,
>+                               state.linkTitle || state.linkURL);
>+    } catch (e) {
>+      return;
>+    }

I wonder if this code should go to BookmarkHelper.js. I guess it could be moved it someone else need it (thinking loud)

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul
>           <richlistitem class="context-command" id="context-share-image" type="image-shareable" onclick="ContextCommands.shareMedia();">
>             <label value="&contextShareImage.label;"/>
>           </richlistitem>
>+          <richlistitem class="context-command" id="context-bookmark-link" type="link" onclick="ContextCommands.bookmarkLink();">
>+            <label value="&contextBookmarkLink.label;"/>
>+          </richlistitem>
>           <richlistitem class="context-command" id="context-play-media" type="media-paused" onclick="ContextCommands.sendCommand('play');">
>             <label value="&contextPlayMedia.label;"/>
>           </richlistitem>

Nit: I notice that on desktop the "Bookmard" row is place before any saving options for the target link, I suggest doing the same here.
Attachment #516690 - Flags: review?(21) → review+
(In reply to comment #7)
> Nit: I notice that on desktop the "Bookmard" row is place before any saving
> options for the target link, I suggest doing the same here.

Done.

http://hg.mozilla.org/mobile-browser/rev/33f136e0b735
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
VERIFIED FIXED on:

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110323 Firefox/4.0b13pre Fennec /4.1a1pre 

Device: Motorola Droid 2 (Android 2.2)
Status: RESOLVED → VERIFIED
Depends on: 646648
You need to log in before you can comment on or make changes to this bug.