Closed
Bug 1009351
Opened 11 years ago
Closed 11 years ago
Add link text to mozbrowser context menu data
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: daleharvey, Assigned: kanru)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [systemsfe])
Attachments
(1 file)
2.15 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → dale
Reporter | ||
Updated•11 years ago
|
Whiteboard: [systemsfe]
Reporter | ||
Comment 1•11 years ago
|
||
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.
Reporter | ||
Comment 2•11 years ago
|
||
Needinfoing Ehsan as you volunteered yourself on the mailing list :)
Flags: needinfo?(ehsan)
Comment 3•11 years ago
|
||
(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)
Reporter | ||
Comment 4•11 years ago
|
||
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
Reporter | ||
Comment 5•11 years ago
|
||
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
Reporter | ||
Comment 6•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
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
Comment 9•11 years ago
|
||
I've confirmed with Kan-Ru that he is the owner of mozbrowser.
Flags: needinfo?(overholt) → needinfo?(kchen)
Reporter | ||
Comment 10•11 years ago
|
||
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)
Comment 11•11 years ago
|
||
Yes, bookmark app is only running when users trigger an activity
Flags: needinfo?(crdlc)
Reporter | ||
Comment 12•11 years ago
|
||
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
Assignee | ||
Comment 13•11 years ago
|
||
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)
Reporter | ||
Comment 14•11 years ago
|
||
textContent will already do that for us I believe, as for limiting, would probably be a sensible idea
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Component: Gaia::System → DOM
Product: Firefox OS → Core
Comment 16•11 years ago
|
||
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+
Assignee | ||
Comment 17•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Reporter | ||
Updated•11 years ago
|
Depends on: rocketbar-search-mvp
Reporter | ||
Updated•11 years ago
|
No longer depends on: rocketbar-search-mvp
Reporter | ||
Updated•11 years ago
|
Blocks: rocketbar-search-mvp
Comment 19•10 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•