Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1008993007:
Fix style on iOS for distilled content. (Closed)
|
Created:
5 years, 9 months ago by noyau (Ping after 24h) Modified:
5 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix style on iOS for distilled content.
On iOS the resource load can't be highjacked to be replaced by local
content: Inlining the necessary resources and fixing the CSS so the page
actually scrolls on iOS.
BUG=None
Committed: https://crrev.com/b5311b2ad302d611309d2ec856289000ef096943
Cr-Commit-Position: refs/heads/master@{#322138}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Style fix #
Total comments: 2
Patch Set 3 : Real fix. #
Messages
Total messages: 18 (3 generated)
noyau@chromium.org changed reviewers: + cjhopman@chromium.org, nyquist@chromium.org
PTAL
https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... File components/dom_distiller/core/viewer.cc (right): https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... components/dom_distiller/core/viewer.cc:118: // and return the local data once a page is loaded. I was under the impression that on iOS we could handle all requests to the chrome-distiller:// scheme ourselves. Is that not the case? Is there documentation somewhere that says what we can/can't do?
https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... File components/dom_distiller/core/html/dom_distiller_viewer.html (right): https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... components/dom_distiller/core/html/dom_distiller_viewer.html:12: <style> Should the <link rel...> be within <style></style> or was this meant to be part of the substitution for iOS only? https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... File components/dom_distiller/core/viewer.cc (right): https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... components/dom_distiller/core/viewer.cc:118: // and return the local data once a page is loaded. On 2015/03/19 at 21:24:52, cjhopman wrote: > I was under the impression that on iOS we could handle all requests to the chrome-distiller:// scheme ourselves. Is that not the case? Is there documentation somewhere that says what we can/can't do? +1 many times. I think at least up to a +4 here. This CL confuses me and makes me a little bit sad.
On 2015/03/19 21:39:16, nyquist wrote: > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > File components/dom_distiller/core/html/dom_distiller_viewer.html (right): > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > components/dom_distiller/core/html/dom_distiller_viewer.html:12: <style> > Should the <link rel...> be within <style></style> or was this meant to be part > of the substitution for iOS only? > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > File components/dom_distiller/core/viewer.cc (right): > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > components/dom_distiller/core/viewer.cc:118: // and return the local data once a > page is loaded. > On 2015/03/19 at 21:24:52, cjhopman wrote: > > I was under the impression that on iOS we could handle all requests to the > chrome-distiller:// scheme ourselves. Is that not the case? Is there > documentation somewhere that says what we can/can't do? > > +1 many times. I think at least up to a +4 here. > > This CL confuses me and makes me a little bit sad. At this point in time a WebState can only be in two modes: - Load content from the app (webUI, interstitials...) - Load content from the network. Reader mode can't run in the first mode (it loads images from the network) so it has to run in the second, insecure, mode. In that insecure mode it is not allowed for the page to load anything locally. There is no API in web/ at this point allowing me to register a dom-distiller scheme and doing what you want to do. So as a stop gap solution this CL works for now.
On 2015/03/20 11:31:20, noyau wrote: > On 2015/03/19 21:39:16, nyquist wrote: > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > File components/dom_distiller/core/html/dom_distiller_viewer.html (right): > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > components/dom_distiller/core/html/dom_distiller_viewer.html:12: <style> > > Should the <link rel...> be within <style></style> or was this meant to be > part > > of the substitution for iOS only? > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > File components/dom_distiller/core/viewer.cc (right): > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > components/dom_distiller/core/viewer.cc:118: // and return the local data once > a > > page is loaded. > > On 2015/03/19 at 21:24:52, cjhopman wrote: > > > I was under the impression that on iOS we could handle all requests to the > > chrome-distiller:// scheme ourselves. Is that not the case? Is there > > documentation somewhere that says what we can/can't do? > > > > +1 many times. I think at least up to a +4 here. > > > > This CL confuses me and makes me a little bit sad. > > At this point in time a WebState can only be in two modes: > > - Load content from the app (webUI, interstitials...) > - Load content from the network. > > Reader mode can't run in the first mode (it loads images from the network) so it > has to run in the second, insecure, mode. In that insecure mode it is not > allowed for the page to load anything locally. > > There is no API in web/ at this point allowing me to register a dom-distiller > scheme and doing what you want to do. So as a stop gap solution this CL works > for now. Can we run in the first mode and fetch network resources explicitly ourselves? We should have a content security policy for the viewer page that disallows inline javascript (can we do that on iOS?). This approach of inlining everything won't work with that (though it could be made to work if there is support for nonces/hashes in the csp).
On 2015/03/23 02:37:12, cjhopman wrote: > On 2015/03/20 11:31:20, noyau wrote: > > On 2015/03/19 21:39:16, nyquist wrote: > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > File components/dom_distiller/core/html/dom_distiller_viewer.html (right): > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > components/dom_distiller/core/html/dom_distiller_viewer.html:12: <style> > > > Should the <link rel...> be within <style></style> or was this meant to be > > part > > > of the substitution for iOS only? > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > File components/dom_distiller/core/viewer.cc (right): > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > components/dom_distiller/core/viewer.cc:118: // and return the local data > once > > a > > > page is loaded. > > > On 2015/03/19 at 21:24:52, cjhopman wrote: > > > > I was under the impression that on iOS we could handle all requests to the > > > chrome-distiller:// scheme ourselves. Is that not the case? Is there > > > documentation somewhere that says what we can/can't do? > > > > > > +1 many times. I think at least up to a +4 here. > > > > > > This CL confuses me and makes me a little bit sad. > > > > At this point in time a WebState can only be in two modes: > > > > - Load content from the app (webUI, interstitials...) > > - Load content from the network. > > > > Reader mode can't run in the first mode (it loads images from the network) so > it > > has to run in the second, insecure, mode. In that insecure mode it is not > > allowed for the page to load anything locally. > > > > There is no API in web/ at this point allowing me to register a dom-distiller > > scheme and doing what you want to do. So as a stop gap solution this CL works > > for now. > > Can we run in the first mode and fetch network resources explicitly ourselves? > Nope. As a web/ client I do not have API to do that. And with WKWebView I'm not sure I'll be able to anyway. > We should have a content security policy for the viewer page that disallows > inline javascript (can we do that on iOS?). This approach of inlining everything > won't work with that (though it could be made to work if there is support for > nonces/hashes in the csp). AFAIK there is no content policy that I know of in iOS. Hence the drastic separation local/remote modes.
On 2015/03/23 12:27:52, noyau wrote: > On 2015/03/23 02:37:12, cjhopman wrote: > > On 2015/03/20 11:31:20, noyau wrote: > > > On 2015/03/19 21:39:16, nyquist wrote: > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > File components/dom_distiller/core/html/dom_distiller_viewer.html (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > components/dom_distiller/core/html/dom_distiller_viewer.html:12: <style> > > > > Should the <link rel...> be within <style></style> or was this meant to be > > > part > > > > of the substitution for iOS only? > > > > > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > File components/dom_distiller/core/viewer.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > components/dom_distiller/core/viewer.cc:118: // and return the local data > > once > > > a > > > > page is loaded. > > > > On 2015/03/19 at 21:24:52, cjhopman wrote: > > > > > I was under the impression that on iOS we could handle all requests to > the > > > > chrome-distiller:// scheme ourselves. Is that not the case? Is there > > > > documentation somewhere that says what we can/can't do? > > > > > > > > +1 many times. I think at least up to a +4 here. > > > > > > > > This CL confuses me and makes me a little bit sad. > > > > > > At this point in time a WebState can only be in two modes: > > > > > > - Load content from the app (webUI, interstitials...) > > > - Load content from the network. > > > > > > Reader mode can't run in the first mode (it loads images from the network) > so > > it > > > has to run in the second, insecure, mode. In that insecure mode it is not > > > allowed for the page to load anything locally. > > > > > > There is no API in web/ at this point allowing me to register a > dom-distiller > > > scheme and doing what you want to do. So as a stop gap solution this CL > works > > > for now. > > > > Can we run in the first mode and fetch network resources explicitly ourselves? > > > Nope. As a web/ client I do not have API to do that. And with WKWebView I'm not > sure I'll be able to anyway. > > > We should have a content security policy for the viewer page that disallows > > inline javascript (can we do that on iOS?). This approach of inlining > everything > > won't work with that (though it could be made to work if there is support for > > nonces/hashes in the csp). > > AFAIK there is no content policy that I know of in iOS. Hence the drastic > separation local/remote modes. I'm fine with this as a short term approach then. I'll start writing up something tomorrow about some of the different things that we want to be able to do when viewing distilled data and we can work out some plans on how we can make things work on ios/non-ios.
On 2015/03/24 07:16:55, cjhopman wrote: > On 2015/03/23 12:27:52, noyau wrote: > > On 2015/03/23 02:37:12, cjhopman wrote: > > > On 2015/03/20 11:31:20, noyau wrote: > > > > On 2015/03/19 21:39:16, nyquist wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > > File components/dom_distiller/core/html/dom_distiller_viewer.html > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > > components/dom_distiller/core/html/dom_distiller_viewer.html:12: <style> > > > > > Should the <link rel...> be within <style></style> or was this meant to > be > > > > part > > > > > of the substitution for iOS only? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > > File components/dom_distiller/core/viewer.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... > > > > > components/dom_distiller/core/viewer.cc:118: // and return the local > data > > > once > > > > a > > > > > page is loaded. > > > > > On 2015/03/19 at 21:24:52, cjhopman wrote: > > > > > > I was under the impression that on iOS we could handle all requests to > > the > > > > > chrome-distiller:// scheme ourselves. Is that not the case? Is there > > > > > documentation somewhere that says what we can/can't do? > > > > > > > > > > +1 many times. I think at least up to a +4 here. > > > > > > > > > > This CL confuses me and makes me a little bit sad. > > > > > > > > At this point in time a WebState can only be in two modes: > > > > > > > > - Load content from the app (webUI, interstitials...) > > > > - Load content from the network. > > > > > > > > Reader mode can't run in the first mode (it loads images from the network) > > so > > > it > > > > has to run in the second, insecure, mode. In that insecure mode it is not > > > > allowed for the page to load anything locally. > > > > > > > > There is no API in web/ at this point allowing me to register a > > dom-distiller > > > > scheme and doing what you want to do. So as a stop gap solution this CL > > works > > > > for now. > > > > > > Can we run in the first mode and fetch network resources explicitly > ourselves? > > > > > Nope. As a web/ client I do not have API to do that. And with WKWebView I'm > not > > sure I'll be able to anyway. > > > > > We should have a content security policy for the viewer page that disallows > > > inline javascript (can we do that on iOS?). This approach of inlining > > everything > > > won't work with that (though it could be made to work if there is support > for > > > nonces/hashes in the csp). > > > > AFAIK there is no content policy that I know of in iOS. Hence the drastic > > separation local/remote modes. > > I'm fine with this as a short term approach then. I'll start writing up > something tomorrow about some of the different things that we want to be able to > do when viewing distilled data and we can work out some plans on how we can make > things work on ios/non-ios. s/short term approach/short term approach (and maybe long-term, too)/
PTAL https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... File components/dom_distiller/core/html/dom_distiller_viewer.html (right): https://codereview.chromium.org/1008993007/diff/1/components/dom_distiller/co... components/dom_distiller/core/html/dom_distiller_viewer.html:12: <style> On 2015/03/19 21:39:15, nyquist wrote: > Should the <link rel...> be within <style></style> or was this meant to be part > of the substitution for iOS only? Yes, my mistake, this is a remnant of my iterations trying on iOS only.
lgtm https://codereview.chromium.org/1008993007/diff/20001/components/dom_distille... File components/dom_distiller/core/viewer.cc (right): https://codereview.chromium.org/1008993007/diff/20001/components/dom_distille... components/dom_distiller/core/viewer.cc:119: substitutions.push_back(viewer::GetCss()); // $2 This now needs <style>...</style>?
https://codereview.chromium.org/1008993007/diff/20001/components/dom_distille... File components/dom_distiller/core/viewer.cc (right): https://codereview.chromium.org/1008993007/diff/20001/components/dom_distille... components/dom_distiller/core/viewer.cc:119: substitutions.push_back(viewer::GetCss()); // $2 On 2015/03/25 02:25:11, cjhopman wrote: > This now needs <style>...</style>? That's what happen when you push code late in the day. You simply forget to add a file before pushing. Duh.
The CQ bit was checked by noyau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1008993007/#ps40001 (title: "Real fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008993007/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b5311b2ad302d611309d2ec856289000ef096943 Cr-Commit-Position: refs/heads/master@{#322138} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
