Closed Bug 1009351 Opened 6 years ago Closed 5 years ago

Add link text to mozbrowser context menu data

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: daleharvey, Assigned: kanru)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [systemsfe])

Attachments

(1 file)

When saving a link to the homescreen, we need some type of description for the link, we should add the text contained within the link to the reported data
Assignee: nobody → dale
Whiteboard: [systemsfe]
So theres another issue with this, we can use the link text as the homescreen title, but it will still have a blank icon, the only way round this I can see is to load the url in the background (possibly in the bookmark app) and fetch the icon, but that seems to be asking a lot (tarako cant really open 2 concurrent browing windows)

The homescreen may be able to xhr index.html and parse it without having to load the external resources, that will significantly reduce the load, but getting into unknown territory here.
Needinfoing Ehsan as you volunteered yourself on the mailing list :)
Flags: needinfo?(ehsan)
(In reply to Dale Harvey (:daleharvey) from comment #2)
> Needinfoing Ehsan as you volunteered yourself on the mailing list :)

Andrew is looking for an owner.
Flags: needinfo?(ehsan) → needinfo?(overholt)
Laying out the options

1. Push back on the feature - People arent as likely to bookmark pages they havent visited, you can save the current page to the homescreen

2. Use link text, blank icon - Easy, really confusing as all app icons will be generic, will be bookmarking a link that possibly redirects, link text may be meaningless

3. As 2, but update blank bookmarks that you visit - Will be fairly complex to update bookmarks, may run into implementation issues, confusing for user as their bookmarks change, I dont think it will work on links that redirect

4. Load link in mozbrowser - Hugely expensive on memory / cpu / network, I think expensive enough to not consider, straight up wont work on tarako

5. Load link url and parse html / dont load external resource - As email, will be complex to implement
So I checked what happens with firefox when we do this

1. The bookmark is always saved with the link text, the text isnt updated when the page is visited
2. If it is an unvisited page, it is stored with a blank icon and the icon will be updated when we visit the page, If we have previously visited the page the original icon will be picked up

I will look into the viability of having bookmarks keep their icons in sync with their equivalent places data
Christian, what do you think about this, off the top of my head

When the homescreen sees a null icon, it tries to read the icon from the places store, it also needs a listener that ensures if a currently bookmarked url is changed in places, it should be updated.

I dont think that should be that complicated, and it looks like picking the link text is enough, so can split those out into seperate bugs.
Flags: needinfo?(crdlc)
I think that we could do it:

When the bookmark is updated in places store we could automatically update this info in bookmarks store and then, homescreen apps will receive an event saying "xx bookmark has been updated" and they could paint the correct icon for free without adding any code in homescreen apps. So we'll avoid that homes have to access to places store thought

Is it feasible or did I miss something?
Flags: needinfo?(crdlc)
That is what I first thought, however will the bookmark app be running? I thought it was mostly only alive whenever people go through the bookmark activities, I would prefer to keep this code out of the homescreen if possible
I've confirmed with Kan-Ru that he is the owner of mozbrowser.
Flags: needinfo?(overholt) → needinfo?(kchen)
Christian

As above, the bookmark app would need to be running in order to see the update to the places store and propogate that change to the homescreen wont and right now it will only run on user triggering activities right?
Flags: needinfo?(crdlc)
Yes, bookmark app is only running when users trigger an activity
Flags: needinfo?(crdlc)
Talked about this with Christian, we can import the shared bookmarks library into system app and have that update the bookmarks manually on page load, the system app can also pick up previously visited icons before triggering the save homescreen activity.

I think we will need to start taking a look at merging the places / bookmarks as Ben has mention, this is duplication of data but more important its extra work being done on every page load which is a performance critical time, but future plans, this will work for now
The link text could be a quite complex sub DOM. We would have to return only the text representation. Do you think we should try to limit the maximum characters returned?
Flags: needinfo?(kchen)
textContent will already do that for us I believe, as for limiting, would probably be a sensible idea
Assignee: dale → kchen
Status: NEW → ASSIGNED
Attachment #8439850 - Flags: review?(bugs)
Component: Gaia::System → DOM
Product: Firefox OS → Core
Comment on attachment 8439850 [details] [diff] [review]
Add link text to mozbrowser context menu data

This is fine, or perhaps we should limit the length of .text, so that we don't
accidentally send huge amount data from child to parent.
elem.textContent.substring(0, 128); or some such?
Attachment #8439850 - Flags: review?(bugs) → review+
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/7490b2dbc69c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
No longer depends on: rocketbar-search-mvp
Blocks: 1007816
I believe the documentation is now complete, see here:

https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowsercontextmenu#The_MenuSystem_object

The text property is what you are on about, right? Let me know if I'm wrong.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.