47 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
47 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
43.72 KB, image/png
39.02 KB, image/png
39 bytes, text/plain
78 bytes, text/plain
4.38 KB, application/zip
Chrome exposes a URL scheme that can be used to open Chrome and make it load a page. I think we want to support something similar so that other developers can include a 'Open web pages in Firefox' option to their application. Maybe we want to extend this a bit more and let other apps do things like: * Open in Firefox * Open in Firefox in Reading Mode I don't think this is a replacement for the Action and Share extensions that we also have. I think it complements those. Many apps have shortcut buttons for actions like 'open in browser' or 'add to my preferred reading list' and that is exactly why we would expose this 'API'.
This is actually something we also want for Android -- deep linking for Twitter. Let's make sure those things agree.
To take this further, I think first we need to finalise the URI schemes Firefox will handle. Chrome handles following: googlechrome for http googlechromes for https googlechrome-x-callback for callbacks May be we need another for open in Firefox in Reading mode? Will these be ok? firefox firefoxs firefox-x-callback firefox-reader
Those sound good to me, but: * We support multiple release channels on Android. We might well do so on iOS. The choice we make here has to allow for that -- can multiple apps consume the same URL schemes? If not, are we happy with 'fennecs:'? * This is a public API, and once settled it's harder to change than other decisions. It would be worth starting a discussion on mobile-firefox-dev to get additional opinions before committing.
(In reply to Richard Newman [:rnewman] from comment #3) > Those sound good to me, but: > > * We support multiple release channels on Android. We might well do so on > iOS. The choice we make here has to allow for that -- can multiple apps > consume the same URL schemes? If not, are we happy with 'fennecs:'? I don't think they can. It is unclear what happens if multiple apps implement the same scheme. Probably random or some undefined order. In reality we will not be able to push a Beta or Nightly channel to the App Store anyway. Apple does not allow it. This is why there also is no Chrome Beta on the App Store. So the audience for this specific feature is *extremely* limited. So I would suggest that we do different schemes and that we create a small 'SDK' that third party apps can include. This SDK will basically wrap UIApplication.canOpenURL()/openURL() and by default just finds and delegates to the most 'stable' version of Firefox installed, with the option to specify a specific version if needed. (But I doubt third party apps would use that since it makes no sense on the App Store. It would be nice for our own testing and experiments though)
I'm a bit surprised this is tracking-... but Chrome has a pretty simple library that devs can use to do this (on their side, this doesn't include the code on our side). We should fork it: https://github.com/GoogleChrome/OpenInChrome/blob/master/OpenInChromeController.m
I think it's tracking- 'cos we wouldn't block ship on it. IMO we definitely want to do this, but we shouldn't rush something that we can't change later -- so let's aim for v1.1/v2.0. (If we can get a good, solid solution done sooner without slipping other goals, then great, of course.)
I would also like to see this included sooner than later. But I think think we should only ship this if we can also convince a bunch of third-party apps to use it. Examples that come to mind: Twitterific, Tweetbot, Reeder. These all support some form of 'open in $BROWSER_OF_CHOICE' if I remember correctly. I can send out some pings to developers and see what they think.
There's more discussion about this recently, and we decided tracking- back when June 2 was the expected ship date. Renominating to be sure this doesn't get overlooked.
Created attachment 8623867 [details] [review] PR (part 1) Beginning of starter implementation for opening links in FFX-iOS
Just a couple of small swift-ifications but otherwise good stuff :)
Comment on attachment 8623867 [details] [review] PR (part 1) LGTM
Comment on attachment 8623867 [details] [review] PR (part 1) I don't want to + this just yet because there are some issues in the PR that need to be addressed. Most important is using NSURLComponents to parse the incoming URL and to not use force unwrapping on NSURL(string:).
Comment on attachment 8623867 [details] [review] PR (part 1) Looks good, please merge.
mock upzz plzz!
Currently working on handling the UI for callbacks to Applications who open links in FFX-iOS and the client/app code for it!
Created attachment 8625963 [details] Back to App.png Here is a quick mock of the back to app UI.
o0o0o0o0o0o0o0o mazin' mazing
Created attachment 8625974 [details] Back to App.png
Created attachment 8626612 [details] Back to App Landscape.png For landscape, it's going to be a bit tricky, but (I think/hope) we can do it. On load, display the caret and label—but disable the rest of the nav. If the user taps on a link within that view, re-enable the nav and disable the caret and label.
Additionally, cap the label to 15 characters and use an ellipsis to truncate.
ooooo byootiful, thanks robin!
Created attachment 8628391 [details] [review] PR (part 2) just want the logic for handling the UI changes to be reviewed and refactored, etc.
Comment on attachment 8628391 [details] [review] PR (part 2) I doubt Stefan will get to this today (Canada day!). Let's see if I manage it.
Created attachment 8628515 [details] Back to App.png updated with new caret asset example, more detail for text label.
Created attachment 8628516 [details] Back to App Landscape.png updated with new caret asset example, more detail for text label.
:bkmunar you nipped out before I had a chance to answer so I'm leaving this here… 15:51 <bkmunar> i know you're on pto sigh, but wanna give me a brief talk about how to include assets with varying sizes? and/or point me to an example in the code? ahha sorry again >.< As far as I know, you use the UIImageView class. Example: In Client/Frontend/Reader/ReaderModeStyleViewController.swift 157 let brightnessMinImageView = UIImageView(image: UIImage(named: "brightnessMin")) 158 brightnessRow.addSubview(brightnessMinImageView) The image is located in ReaderSettings.xcassets Xcode now supports Asset Catalogs, so all you have to do is drag them here and it will automatically render a .json with the calls for the varying sizes. At first they will display below and you'll see three boxes [@1x][@2x][@3x] to drag them in. Make sure they are assigned "Universal" and you should be good to go! To make a new Asset Catalog, just find another .xcassets file and click the [+] on the bottom left of the asset list column. (at least, this is the easiest way I can find to do it)
Comment on attachment 8628391 [details] [review] PR (part 2) Feedback in the PR.
Comment on attachment 8628391 [details] [review] PR (part 2) revised code
Created attachment 8629015 [details] Code for 3rd party SDKs, please review! "OpenInChromeAndFirefoxController"
Comment on attachment 8629015 [details] Code for 3rd party SDKs, please review! "OpenInChromeAndFirefoxController" I don't like this for several reasons. First this code starts with "Copyright 2012-2014, Google Inc". It has their license on top. Which is really odd. Second, by supporting both Chrome and Firefox we now have become the maintainers of the Chrome code too. This is not what we want. My suggestion is to create a new project from scratch with three files in it: * OpenInFirefox.swift * OpenInFirefox.h * OpenInFirefox.m Both implementations do the same. One is for Swift projects, the other for Objective-C projects. The reason for providing both a Swift and Objective-C implementation is to make it simpler for developers to include their preferred implementation language.
St3fan: ok! I have the code for that too which I shall upload; My assumption to what Richard meant by adding Firefox support for the chrome code too was to fork it and add it in? So I did that and have my custom files too that are what you specified above. Thanks!
For reference, look at my OpenInFirefoxClient project on github, that has the code there (but needs to be updated a bit and will get to)
Quick question – is there a reason we use a custom back caret vs. the system supplied one? I don't think we use a custom caret anywhere else, do we?
Darrin, it's because urlBar doesn't use the one apple provides (navigation toolbar or something?) which provides the back Caret for you, so we need to make a custom one (I believe)
Aka, if we don't use that class apple provides, we don't have access to that caret
:darrin - I prefer the native asset but Bryan ran into the above issue. I at least rounded it out a little to match the rest of the UI.
Comment on attachment 8623867 [details] [review] PR (part 1) Just for future reference, once the PR is merged and the bug is marked as RESOLVED/FIXED, it's best to open a new bug to address any follow-ups. Reopening the same bug for multiple PRs adds clutter and is harder to follow.
Comment on attachment 8629459 [details] 3rd party code https://github.com/bkmunar/OpenInFirefoxClient/pull/2
For the third party code, I created a new mozilla repository that we can move this to once r+'d: https://github.com/mozilla/firefox-ios-open-in-client
:bkmunar when in landscape, the navigation is disabled. The back caret and label display. If the user taps anything in the webview, the label disappears and becomes a back arrow. If the user goes back, the caret and label is displayed *and* the forward arrow. I'm making a new back caret that is essentially the back arrow without the line. If this doesn't make sense let me know and we can hop on a vidyo chat.
Created attachment 8630802 [details] backCaret.zip updated backCaret
Comment on attachment 8629459 [details] 3rd party code https://github.com/bkmunar/OpenInFirefoxClient/pull/3/files
Comment on attachment 8629459 [details] 3rd party code I removed myself from feedback to clear my queue. But if this patch is finished and you want a second pair of eyes on it, please add me back.
Comment on attachment 8629459 [details] 3rd party code 3rd party PR looks OK to me. Can you squash all of these into a single commit before landing?
Comment on attachment 8628391 [details] [review] PR (part 2) Clearing review until new version is ready.
Done for v1 for now, will file separate bugs for v1.1 and on