Closed Bug 960813 Opened 6 years ago Closed 6 years ago

[B2G][Browser][RTSP] Unable to add RTSP bookmark to the "Home screen"

Categories

(Firefox OS Graveyard :: Gaia::Browser, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v1.3 affected, b2g-v1.3T affected)

RESOLVED FIXED
1.4 S1 (14feb)
Tracking Status
b2g-v1.3 --- affected
b2g-v1.3T --- affected

People

(Reporter: sarsenyev, Assigned: dietrich)

References

Details

(Whiteboard: burirun1.3-2, 1.3tarakorun2, [systemsfe])

Attachments

(3 files)

Attached video videobug2.mp4
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?
Flags: needinfo?(sarsenyev)
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
Flags: needinfo?(sarsenyev)
Sounds like a broken experience then on a 1.3 committed feature.
blocking-b2g: --- → 1.3?
blocking-b2g: 1.3? → 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.
Assignee: nobody → dietrich
WIP patch for feedback.

* allows user to bookmark anything that's a valid URL
* however, allowing non-URLs so need to figure that out ;)
Attachment #8368198 - Flags: review?(dale)
* 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
Attachment #8368198 - Flags: review?(dale)
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.
Attachment #8368198 - Attachment description: WIP - Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846 → Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15846
Attachment #8368198 - Flags: review?(dale)
Could you clarify the STR that you are solving here? there is quite a few quirks around browser bookmarking / homescreen and want to know what you are going for in terms of use case + expected behaviour
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]
Target Milestone: --- → 1.4 S1 (14feb)
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+
https://hg.mozilla.org/mozilla-central/rev/3993618b1a34
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Dale Harvey (:daleharvey) from comment #21)
> 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

This problem occured with or without this patch, with error:

JavaScript error: http://browser.gaiamobile.org:8080/js/browser_db.js, line 221: NS_ERROR_DOM_BAD_URI: Access to restricted URI denied

I suspected that maybe the neterror chrome URL was what was being passed.

HOWEVER...

In the latest build, this no longer occurs because the new browser error pages are modal! You have a choice of close or try again, so can never get to the point at which you can bookmark an invalid URL.

The STR of comment #0 of this bug are still valid however, since you can change the URL when creating a homescreen bookmark. So this bug is still present, requiring this patch.
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
Flags: needinfo?(dietrich)
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.
Flags: needinfo?(whuang)
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.
Attachment #8376092 - Flags: approval-gaia-v1.3?(fabrice)
(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
Attachment #8376092 - Flags: approval-gaia-v1.3?(fabrice)
Depends on: 978371
Whiteboard: burirun1.3-2, [systemsfe] → burirun1.3-2, 1.3tarakorun2, [systemsfe]
You need to log in before you can comment on or make changes to this bug.