Closed Bug 960813 Opened 6 years ago Closed 6 years ago
[B2G][Browser][RTSP] Unable to add RTSP bookmark to the "Home screen"
4.51 MB, video/mp4
46 bytes, text/x-github-pull-request
|Details | Review|
46 bytes, text/x-github-pull-request
|Details | Review|
Description: When adding a bookmark with RTSP link, the bookmark is not added Repro Steps: 1) Updated Buri to BuildID: 20140115004003 2) Launch the browser visit Mozilla website 3) Tap "Star" icon to add it to bookmark 4) Input a RTSP link and save it 5) Tap "Add to homescreen" Actual: The bookmark is not added, the screen with option closes Expected: The bookmark is added to the home screen Environmental Variables: Device: Buri 1.3 MOZ BuildID: 20140115004003 Gaia: 14e199d6a9ad917eacad883820a9f7619dbf42c8 Gecko: d7260b206e91 Version: 28.0a2 Firmware Version: 1.2-device.cfg Notes: Repro frequency: 100% https://moztrap.mozilla.org/manage/cases/?filter-id=11041 See attached: video file
The STR is very unclear here - it's intermixing discussion of bookmarking vs. adding to the homescreen. What's the bug focusing on here?
When RTSP stream opens, there is no option (star) to add to home screen, the option is available when this stream is added as bookmark first, then menu with options are available "add to homescreen or "unbookmark" , so when a user chooses add to homescreen, nothing happens Scenario is when the user doesn't want anymore to open browser to see the previously saved RTSP link as bookmark, so he is decided to add it to the homesceen
Sounds like a broken experience then on a 1.3 committed feature.
blocking-b2g: --- → 1.3?
Ben, Dale any idea whats going on here?
Whiteboard: burirun1.3-2 → burirun1.3-2, [systemsfe]
The particular steps to reproduce are broken, however going to http://www.webmfiles.org/demo-files/ and attempting to save one of the video files to the homescreen does break
(In reply to Dale Harvey (:daleharvey) from comment #5) > The particular steps to reproduce are broken, however going to > > http://www.webmfiles.org/demo-files/ > > and attempting to save one of the video files to the homescreen does break I have a test site here - http://mozilla.github.io/qa-testcase-data/Video/youtubeRTSPURLSearch.html btw you could use for testing this. Just search for something & you'll get RTSP videos for the top 50 search results from YouTube.
WIP patch for feedback. * allows user to bookmark anything that's a valid URL * however, allowing non-URLs so need to figure that out ;)
* add tests for saving URLs with non http/https schemes as bookmarks to homescreen * add test for bookmarking non-URLs
Latest version contains: * enables saving any valid URL as a homescreen bookmark (now matches browser bookmark behaviour) * adds live validation of URL at browser/homescreen bookmark creation time * adds live validation of URL when editing a browser bookmark Need to add tests per comment #8
Comment on attachment 8368198 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846 Added some comments, theres some existing code to work around that doesnt make a ton of sense, but the bookmark fixes look good For WIP patches dont we f? instead of r?, clearing though
PR now has more tests. I added some browser tests that should be failing (but are passing), and the pre-existing homescreen bookmark tests don't actually save a bookmark (and can't!).
Comment on attachment 8368198 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846 Some more cleanup, and found a way to unit test the UI part at least.
Yeah I've gone through the bookmarking code quite a bit now, definitely could be made simpler. But for now, the substantive changes in this patch: * Updates all bookmark add/edit UI to check that the URL is valid. * Those forms now only allow bookmark to be saved if the URL is valid. * Updated the bookmark-to-homescreen activity to allow any valid URL, instead of just HTTP(S). * Added a regression test to browser db tests to ensure non-HTTP(S) URLs are saved. Other minor changes in this patch, to pre-existing code: * Noted a couple of places where we silently fail. Can add proper roundtrip error handling later. * Logged an unhandled failure. Can add proper roundtrip error handling later. * Moved a few files out of xfail.list, along with the changes needed to pass jshint. * Added a couple of stub tests to browser db tests which are currently passing but should fail (empty title, invalid URL).
Also, I just found a Homescreen integration test which is currently commented out. I'm going to try and get it running, but we don't need to hold off on landing this blocker fix on that.
Can we get ETA to fix this bug? Thank you.
Patch needs review, then can land.
Whiteboard: burirun1.3-2, [systemsfe] → burirun1.3-2, [systemsfe][eta: waiting on review]
I can see whats changed, but its the steps to reproduce I need, like in terms of opening the browser what do you do for a santity test (or tests), its also gonna help QA when it comes to verifying it. Things like 'valid url' are fuzzy terms and on testing this patch I am seeing a lot of behaviour that I might expect this patch to change to not effect anything, (also clarifying between homescreen and browser when you mention bookmarks would be handy) Cheers
STR: Save any non-HTTP(S) URL as a bookmark to Homescreen. Valid URL shouldn't be fuzzy, is defined here: http://mxr.mozilla.org/gaia/source/shared/js/url_helper.js#18 Re homescreen vs browser bookmark - this patch results in URLs being treated consistently, regardless of homescreen and browser (those URLs sourced from same place - browser!).
(In reply to Dietrich Ayala (:dietrich) from comment #15) > Also, I just found a Homescreen integration test which is currently > commented out. I'm going to try and get it running, but we don't need to > hold off on landing this blocker fix on that. bug 952610
Sorry but save url as homescreen isnt steps to reproduce "Open browser, enter hddp://fakedomain then save to homescreen, see that this invalid url can be saved as a bookmark" Something that the bug reporter, qa, the dev and the reviewer can follow exactly, reproduce and use to agree whether its a bug and whether its been fixed Whether a url is valid or not is very fuzzy, we have bugs open against our implementation of html5 url validation, there are open questions about whether we should load app:// url or various other schemes. The reason I wanted the STR as my one ^ failed silently in both master and your patch, which I would consider a bug, but it may not be the bug you were fixing
Dale, comment #0 has detailed STR too ;) Thanks for the information about that URL failing silently! I have that in the unit tests, which passed... so I'll hit those again to see what's up.
(In reply to Dale Harvey (:daleharvey) from comment #21) > Whether a url is valid or not is very fuzzy, we have bugs open against our > implementation of html5 url validation, there are open questions about > whether we should load app:// url or various other schemes. Hehe, so our operating system might not have decided what a URL is (bizaarro), but we don't have to solve that in this patch. This patch just needs to be able to use the *currently-used definition* of a valid URL as a homescreen bookmark, instead of restricting to HTTP(S) only.
Comment on attachment 8368198 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846 Looks good, thanks, and sorry for the delay
Attachment #8368198 - Flags: review?(dale) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: burirun1.3-2, [systemsfe][eta: waiting on review] → burirun1.3-2, [systemsfe]
I was not able to uplift this bug to v1.3. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1.3 git cherry-pick -x 9d7ec082e7e7c7db879a8b1d98ea5dd4cd3155bb <RESOLVE MERGE CONFLICTS> git commit
1.3 blockers no longer have auto-approval to land. Please request gaia-v1.3 approval on the patch for uplift.
I heard RTSP is no longer shipping in 1.3. If that's the case, this bug is no longer needed on the branch. Who can confirm this is the case? Kevin, do you know?
Flags: needinfo?(dietrich) → needinfo?(khu)
Wesley, can you help? Thanks.
Flags: needinfo?(khu) → needinfo?(whuang)
To clarify, RTSP in 1.3 still supports audio streaming. It's the video streaming being disabled.
Ok, thanks Wesley. Rebased patch coming up!
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] I'm not sure why this was considered a blocker - do users bookmark RTSP URLs? [Bug caused by] (feature/regressing bug #): None, RTSP is new in 1.3. [User impact] if declined: User cannot bookmark RTSP URLs. [Testing completed]: See unit tests in patch. Also added validation in the UI. [Risk to taking this patch] (and alternatives if risky): Low risk. [String changes made]: None.
(In reply to Dietrich Ayala (:dietrich) from comment #34) > Created attachment 8376092 [details] [review] > Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16281 > > NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to > better understand the B2G approval process and landings. > > [Approval Request Comment] > I'm not sure why this was considered a blocker - do users bookmark RTSP URLs? Now that I think about this more - I think you are right. The use case here isn't realistic because: The only way you could bookmark a RTSP URL right now is by providing the direct URL during the add bookmark screen. It's not possible ever bookmark the RTSP URL through the star button ever, as don't render RTSP media content in the browser - it renders in the video app. The average joe user probably isn't going through the painful effort of trying to type out an exact RTSP URL into the add bookmark UI, as it's too much effort. Given those factors, I think we pull this blocking list. Sorry I didn't think about this earlier. I realize now that this doesn't really make sense to take to 1.3.
blocking-b2g: 1.3+ → ---
Comment on attachment 8376092 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16281 clearing since it's not wanted on 1.3 anymore
You need to log in before you can comment on or make changes to this bug.