Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
798723004:
Fix copying from interstitial pages on OSX. (Closed)
|
Created:
5 years, 11 months ago by lgarron Modified:
5 years, 7 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, nona+watch_chromium.org, penghuang+watch_chromium.org, James Su, yukishiino+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix copying from interstitial pages on OSX.
BUG=415784
Patch Set 1 #
Total comments: 2
Patch Set 2 : Verified test. #
Total comments: 8
Patch Set 3 : Use web_contents() instead of exposing a new public function. #Patch Set 4 : Address more comments. #
Total comments: 4
Messages
Total messages: 26 (6 generated)
Here is the a simplified fix that allows restores (enables?) from interstitials on OSX.
felt@chromium.org changed reviewers: + felt@chromium.org
did you mean to set reviewers?
On 2015/01/12 at 17:43:11, felt wrote: > did you mean to set reviewers? Thanks for asking, but I think nasko@ wanted me to a test first.
palmer@chromium.org changed reviewers: + palmer@chromium.org
You should finish this up as soon as possible, so that e.g. Mac users can copy and paste bad certificate chains when they get a key pinning validation failure. You'll need to rebase this CL, since it's so old. (I reapplied it manually; "git apply" wouldn't take it.) https://codereview.chromium.org/798723004/diff/1/content/browser/frame_host/i... File content/browser/frame_host/interstitial_page_impl.h (right): https://codereview.chromium.org/798723004/diff/1/content/browser/frame_host/i... content/browser/frame_host/interstitial_page_impl.h:99: FrameTree* GetFrameTree() override; Is moving this from lower down in the file to here the right thing to do? https://codereview.chromium.org/798723004/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/798723004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:631: (InterstitialPageImpl*)GetInterstitialPage(); Use a C++-style cast here. I don't know exactly what nasko wanted as a test, but it might be good to use |TestInterstitialPage| in content/browser/web_contents/web_contents_impl_unittest.cc in a test/test suite.
Patchset #2 (id:20001) has been deleted
lgarron@chromium.org changed reviewers: + nasko@chromium.org
nasko@: With a bit of help from Chris, I now have a simple test that checks for the null pointer. Could you take a look at this again, and see if the whole thing is good to commit? (I've checked that it fails without the fixed version of GetFocusedFrame().)
Thanks for reviving this Lucas. Few comments. https://codereview.chromium.org/798723004/diff/40001/content/browser/frame_ho... File content/browser/frame_host/interstitial_page_impl.h (right): https://codereview.chromium.org/798723004/diff/40001/content/browser/frame_ho... content/browser/frame_host/interstitial_page_impl.h:66: WebContents* GetAsWebContents() override; This doesn't seem right. This is not a WebContents type object, so GetAsWebContents isn't meaningful. https://codereview.chromium.org/798723004/diff/40001/content/browser/frame_ho... content/browser/frame_host/interstitial_page_impl.h:93: FrameTree* GetFrameTree() override; Why do you need to expose this? Can't we use GetMainFrame() whenever this is needed? https://codereview.chromium.org/798723004/diff/40001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/798723004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl_unittest.cc:2295: EXPECT_NE(nullptr, interstitial->GetAsWebContents()->GetFocusedFrame()); Let's add accessor that exposes the associated WebContents in TestInterstitialPage. This way it is only usable in tests and we avoid APIs on the original object that can be misused. https://codereview.chromium.org/798723004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl_unittest.cc:2296: I think we can another expectation here. The RenderFrameHost we get back from WebContents::GetFocusedFrame() while an interstitial page is displayed should be different than WebContents::GetMainFrame(), not just non-null.
lgarron@chromium.org changed reviewers: - felt@chromium.org
https://codereview.chromium.org/798723004/diff/40001/content/browser/frame_ho... File content/browser/frame_host/interstitial_page_impl.h (right): https://codereview.chromium.org/798723004/diff/40001/content/browser/frame_ho... content/browser/frame_host/interstitial_page_impl.h:66: WebContents* GetAsWebContents() override; On 2015/04/22 at 05:07:45, nasko (very slow until 04-27) wrote: > This doesn't seem right. This is not a WebContents type object, so GetAsWebContents isn't meaningful. I think we have to do this to overwrite RenderViewHostDelegate::GetAsWebContents(), which returns NULL. I can't get this CL to work without it. https://codereview.chromium.org/798723004/diff/40001/content/browser/frame_ho... content/browser/frame_host/interstitial_page_impl.h:93: FrameTree* GetFrameTree() override; On 2015/04/22 at 05:07:45, nasko (very slow until 04-27) wrote: > Why do you need to expose this? Can't we use GetMainFrame() whenever this is needed? This is what we came up with when we originally made this CL. :-P Replacing interstitial_page->GetFrameTree()->root()->current_frame_host() in WebContentsImpl::GetFocusedFrame()) with interstitial_page->GetMainFrame() works. Is that safe? https://codereview.chromium.org/798723004/diff/40001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/798723004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl_unittest.cc:2295: EXPECT_NE(nullptr, interstitial->GetAsWebContents()->GetFocusedFrame()); On 2015/04/22 at 05:07:45, nasko (very slow until 04-27) wrote: > Let's add accessor that exposes the associated WebContents in TestInterstitialPage. This way it is only usable in tests and we avoid APIs on the original object that can be misused. As of a week ago, InterstitialPageImpl has a protected web_contents() function that makes this easy, but given that we need GetAsWebContents() [see above] to be a public override I think it makes sense to keep this? https://codereview.chromium.org/798723004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl_unittest.cc:2296: On 2015/04/22 at 05:07:45, nasko (very slow until 04-27) wrote: > I think we can another expectation here. The RenderFrameHost we get back from WebContents::GetFocusedFrame() while an interstitial page is displayed should be different than WebContents::GetMainFrame(), not just non-null. Done.
creis@chromium.org changed reviewers: + creis@chromium.org
I'm concerned about this. This CL essentially papers over the fact that the interstitial is an overlay, making it easy for more callers to not notice the difference (in security level) between the actual page in the WebContents and the page in the interstitial's overlay. We've had security bugs as result of this type of thing in other places (e.g., dialogs). I'm certainly looking forward to having interstitial pages not be an overlay anymore, but I wonder if there's a different way to fix this bug in the meantime. Maybe something like having InterstitialPageImpl handle the messages you need in OnMessageReceived, or checking whether an interstitial is showing in some of the relevant call sites? It's possible that this may be the way to go, but I'd like to consider alternatives. Nasko and I plan to chat about it in person tomorrow. https://codereview.chromium.org/798723004/diff/80001/content/browser/frame_ho... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/798723004/diff/80001/content/browser/frame_ho... content/browser/frame_host/interstitial_page_impl.cc:704: return web_contents_; This feels super risky to me. If callers hear something from an interstitial RenderFrameHost and assume it's from the WebContents, it's really easy for them to confuse the interstitial's URL or process with the last committed URL or process in the WebContents. That could be a big security bug if the last committed page is a WebUI and the interstitial is for an untrusted page. https://codereview.chromium.org/798723004/diff/80001/content/browser/frame_ho... File content/browser/frame_host/interstitial_page_impl.h (right): https://codereview.chromium.org/798723004/diff/80001/content/browser/frame_ho... content/browser/frame_host/interstitial_page_impl.h:66: WebContents* GetAsWebContents() override; This is not defined in InterstitialPage, so it doesn't belong in this section (under line 59). Are you overriding it from RenderFrameHostDelegate? https://codereview.chromium.org/798723004/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/798723004/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:651: return interstitial_page->GetMainFrame(); This is mildly scary to me. We're returning a frame that is not in the frame tree of the WebContents, which could have undefined behavior in any call sites that make that assumption. Maybe it's reasonable since this is specific to focus and kind of makes sense conceptually, but it feels really risky to me.
https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:1413: return WebContents::FromRenderFrameHost( I'm looking at some of the GetAsWebContents() call sites within this class, and some of them seem like they would be wrong with your current change. (There's no cross-linking for Mac in codesearch, so I'm not sure if this function has other call sites that might be broken as well.) For example, GetRenderWidgetHostViewToUse looks wrong. Given a RWHVM, this returns it unless there's a full screen guest (e.g., for signin). However, if we call it with an interstitial's RWHVM (e.g., if we navigate to a broken HTTPS tab while signin is showing), we would expect to get back the interstitial's RWVHM but instead we'd get the (now hidden) guest's RWHVM. As a result, startSpeaking and stopSpeaking would read hidden text instead of what's showing on the interstitial. undo, redo, cut, copy, etc admittedly look like they need to send InputMsgs to the focused frame of the interstitial page if an interstitial is showing. Going through WebContents and changing GetFocusedFrame might work, but I feel like there's probably a better way to handle it than causing this confusion between the WebContents and interstitial. Here's a proposal... What if undo, redo, etc called methods on the RenderWidgetHostDelegate of render_widget_host_? On WebContents, this would use the existing Undo, Redo, etc methods that pass IPCs to its focused frame. On InterstitialPageImpl, this would do essentially the same thing but using the *interstitial page's* frame_tree_.GetFocusedFrame(). We might end up with some code duplication for those simple methods, and we might have to expose RWHI's delegate, but we would end up sending the commands to the right places and not have any new places to worry about confusing the interstitial and the WebContents. Would that work?
On 2015/04/28 at 22:37:27, creis wrote: > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:1413: return WebContents::FromRenderFrameHost( > I'm looking at some of the GetAsWebContents() call sites within this class, and some of them seem like they would be wrong with your current change. (There's no cross-linking for Mac in codesearch, so I'm not sure if this function has other call sites that might be broken as well.) > > For example, GetRenderWidgetHostViewToUse looks wrong. Given a RWHVM, this returns it unless there's a full screen guest (e.g., for signin). However, if we call it with an interstitial's RWHVM (e.g., if we navigate to a broken HTTPS tab while signin is showing), we would expect to get back the interstitial's RWVHM but instead we'd get the (now hidden) guest's RWHVM. As a result, startSpeaking and stopSpeaking would read hidden text instead of what's showing on the interstitial. > > undo, redo, cut, copy, etc admittedly look like they need to send InputMsgs to the focused frame of the interstitial page if an interstitial is showing. Going through WebContents and changing GetFocusedFrame might work, but I feel like there's probably a better way to handle it than causing this confusion between the WebContents and interstitial. > > Here's a proposal... What if undo, redo, etc called methods on the RenderWidgetHostDelegate of render_widget_host_? On WebContents, this would use the existing Undo, Redo, etc methods that pass IPCs to its focused frame. On InterstitialPageImpl, this would do essentially the same thing but using the *interstitial page's* frame_tree_.GetFocusedFrame(). > > We might end up with some code duplication for those simple methods, and we might have to expose RWHI's delegate, but we would end up sending the commands to the right places and not have any new places to worry about confusing the interstitial and the WebContents. Would that work? Nasko and I spent a while last week trying to implement a different fix: @@ -249,7 +249,22 @@ WebContents* WebContents::FromRenderFrameHost(RenderFrameHost* rfh) { RenderFrameHostImpl* rfh_impl = static_cast<RenderFrameHostImpl*>(rfh); if (!rfh_impl) return NULL; - return rfh_impl->delegate()->GetAsWebContents(); + WebContents* web_contents = rfh_impl->delegate()->GetAsWebContents(); + if (web_contents == nullptr) { + InterstitialPageImpl* interstitial_page = + static_cast<InterstitialPageImpl*>(rfh_impl->delegate()); + if (interstitial_page->GetMainFrame() == rfh) { + web_contents = interstitial_page->web_contents(); + DCHECK(web_contents->ShowingInterstitialPage()); + } + } + return web_contents; } That didn't work, and Nasko said he'd think about it. I'm happy to do something more "right", as long as it results in being able to copy from interstitials soon. :-P
On 2015/04/29 04:13:45, lgarron wrote: > On 2015/04/28 at 22:37:27, creis wrote: > > > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > > > > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_mac.mm:1413: return > WebContents::FromRenderFrameHost( > > I'm looking at some of the GetAsWebContents() call sites within this class, > and some of them seem like they would be wrong with your current change. > (There's no cross-linking for Mac in codesearch, so I'm not sure if this > function has other call sites that might be broken as well.) > > > > For example, GetRenderWidgetHostViewToUse looks wrong. Given a RWHVM, this > returns it unless there's a full screen guest (e.g., for signin). However, if > we call it with an interstitial's RWHVM (e.g., if we navigate to a broken HTTPS > tab while signin is showing), we would expect to get back the interstitial's > RWVHM but instead we'd get the (now hidden) guest's RWHVM. As a result, > startSpeaking and stopSpeaking would read hidden text instead of what's showing > on the interstitial. > > > > undo, redo, cut, copy, etc admittedly look like they need to send InputMsgs to > the focused frame of the interstitial page if an interstitial is showing. Going > through WebContents and changing GetFocusedFrame might work, but I feel like > there's probably a better way to handle it than causing this confusion between > the WebContents and interstitial. > > > > Here's a proposal... What if undo, redo, etc called methods on the > RenderWidgetHostDelegate of render_widget_host_? On WebContents, this would use > the existing Undo, Redo, etc methods that pass IPCs to its focused frame. On > InterstitialPageImpl, this would do essentially the same thing but using the > *interstitial page's* frame_tree_.GetFocusedFrame(). > > > > We might end up with some code duplication for those simple methods, and we > might have to expose RWHI's delegate, but we would end up sending the commands > to the right places and not have any new places to worry about confusing the > interstitial and the WebContents. Would that work? > > Nasko and I spent a while last week trying to implement a different fix: > > @@ -249,7 +249,22 @@ WebContents* > WebContents::FromRenderFrameHost(RenderFrameHost* rfh) { RenderFrameHostImpl* > rfh_impl = static_cast<RenderFrameHostImpl*>(rfh); > if (!rfh_impl) > return NULL; > - return rfh_impl->delegate()->GetAsWebContents(); > + WebContents* web_contents = rfh_impl->delegate()->GetAsWebContents(); > + if (web_contents == nullptr) { > + InterstitialPageImpl* interstitial_page = > + static_cast<InterstitialPageImpl*>(rfh_impl->delegate()); > + if (interstitial_page->GetMainFrame() == rfh) { > + web_contents = interstitial_page->web_contents(); > + DCHECK(web_contents->ShowingInterstitialPage()); > + } > + } > + return web_contents; > } > > > That didn't work, and Nasko said he'd think about it. > > I'm happy to do something more "right", as long as it results in being able to > copy from interstitials soon. :-P Ah, that fix isn't safe. We can't assume the delegate is an InterstitialPageImpl just because GetAsWebContents returns null. I think it's worth giving the delegate approach a try, because then RWHVM will tell the correct object to do the copy operation for it.
On 2015/04/29 05:18:54, Charlie Reis wrote: > On 2015/04/29 04:13:45, lgarron wrote: > > On 2015/04/28 at 22:37:27, creis wrote: > > > > > > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > > > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > > > > > > > > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > > > content/browser/renderer_host/render_widget_host_view_mac.mm:1413: return > > WebContents::FromRenderFrameHost( > > > I'm looking at some of the GetAsWebContents() call sites within this class, > > and some of them seem like they would be wrong with your current change. > > (There's no cross-linking for Mac in codesearch, so I'm not sure if this > > function has other call sites that might be broken as well.) > > > > > > For example, GetRenderWidgetHostViewToUse looks wrong. Given a RWHVM, this > > returns it unless there's a full screen guest (e.g., for signin). However, if > > we call it with an interstitial's RWHVM (e.g., if we navigate to a broken > HTTPS > > tab while signin is showing), we would expect to get back the interstitial's > > RWVHM but instead we'd get the (now hidden) guest's RWHVM. As a result, > > startSpeaking and stopSpeaking would read hidden text instead of what's > showing > > on the interstitial. > > > > > > undo, redo, cut, copy, etc admittedly look like they need to send InputMsgs > to > > the focused frame of the interstitial page if an interstitial is showing. > Going > > through WebContents and changing GetFocusedFrame might work, but I feel like > > there's probably a better way to handle it than causing this confusion between > > the WebContents and interstitial. > > > > > > Here's a proposal... What if undo, redo, etc called methods on the > > RenderWidgetHostDelegate of render_widget_host_? On WebContents, this would > use > > the existing Undo, Redo, etc methods that pass IPCs to its focused frame. On > > InterstitialPageImpl, this would do essentially the same thing but using the > > *interstitial page's* frame_tree_.GetFocusedFrame(). > > > > > > We might end up with some code duplication for those simple methods, and we > > might have to expose RWHI's delegate, but we would end up sending the commands > > to the right places and not have any new places to worry about confusing the > > interstitial and the WebContents. Would that work? > > > > Nasko and I spent a while last week trying to implement a different fix: > > > > @@ -249,7 +249,22 @@ WebContents* > > WebContents::FromRenderFrameHost(RenderFrameHost* rfh) { > RenderFrameHostImpl* > > rfh_impl = static_cast<RenderFrameHostImpl*>(rfh); > > if (!rfh_impl) > > return NULL; > > - return rfh_impl->delegate()->GetAsWebContents(); > > + WebContents* web_contents = rfh_impl->delegate()->GetAsWebContents(); > > + if (web_contents == nullptr) { > > + InterstitialPageImpl* interstitial_page = > > + static_cast<InterstitialPageImpl*>(rfh_impl->delegate()); > > + if (interstitial_page->GetMainFrame() == rfh) { > > + web_contents = interstitial_page->web_contents(); > > + DCHECK(web_contents->ShowingInterstitialPage()); > > + } > > + } > > + return web_contents; > > } > > > > > > That didn't work, and Nasko said he'd think about it. > > > > I'm happy to do something more "right", as long as it results in being able to > > copy from interstitials soon. :-P > > Ah, that fix isn't safe. We can't assume the delegate is an > InterstitialPageImpl just because GetAsWebContents returns null. > > I think it's worth giving the delegate approach a try, because then RWHVM will > tell the correct object to do the copy operation for it. Also, Charlie and I already discussed several options, including the hack we attempted when I was in MTV. The conclusion was that Charlie's proposal was the cleanest of all.
On 2015/04/29 at 06:03:34, nasko wrote: > On 2015/04/29 05:18:54, Charlie Reis wrote: > > On 2015/04/29 04:13:45, lgarron wrote: > > > On 2015/04/28 at 22:37:27, creis wrote: > > > > > > > > > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > > > > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > > > > > > > > > > > > https://codereview.chromium.org/798723004/diff/80001/content/browser/renderer... > > > > content/browser/renderer_host/render_widget_host_view_mac.mm:1413: return > > > WebContents::FromRenderFrameHost( > > > > I'm looking at some of the GetAsWebContents() call sites within this class, > > > and some of them seem like they would be wrong with your current change. > > > (There's no cross-linking for Mac in codesearch, so I'm not sure if this > > > function has other call sites that might be broken as well.) > > > > > > > > For example, GetRenderWidgetHostViewToUse looks wrong. Given a RWHVM, this > > > returns it unless there's a full screen guest (e.g., for signin). However, if > > > we call it with an interstitial's RWHVM (e.g., if we navigate to a broken > > HTTPS > > > tab while signin is showing), we would expect to get back the interstitial's > > > RWVHM but instead we'd get the (now hidden) guest's RWHVM. As a result, > > > startSpeaking and stopSpeaking would read hidden text instead of what's > > showing > > > on the interstitial. > > > > > > > > undo, redo, cut, copy, etc admittedly look like they need to send InputMsgs > > to > > > the focused frame of the interstitial page if an interstitial is showing. > > Going > > > through WebContents and changing GetFocusedFrame might work, but I feel like > > > there's probably a better way to handle it than causing this confusion between > > > the WebContents and interstitial. > > > > > > > > Here's a proposal... What if undo, redo, etc called methods on the > > > RenderWidgetHostDelegate of render_widget_host_? On WebContents, this would > > use > > > the existing Undo, Redo, etc methods that pass IPCs to its focused frame. On > > > InterstitialPageImpl, this would do essentially the same thing but using the > > > *interstitial page's* frame_tree_.GetFocusedFrame(). > > > > > > > > We might end up with some code duplication for those simple methods, and we > > > might have to expose RWHI's delegate, but we would end up sending the commands > > > to the right places and not have any new places to worry about confusing the > > > interstitial and the WebContents. Would that work? > > > > > > Nasko and I spent a while last week trying to implement a different fix: > > > > > > @@ -249,7 +249,22 @@ WebContents* > > > WebContents::FromRenderFrameHost(RenderFrameHost* rfh) { > > RenderFrameHostImpl* > > > rfh_impl = static_cast<RenderFrameHostImpl*>(rfh); > > > if (!rfh_impl) > > > return NULL; > > > - return rfh_impl->delegate()->GetAsWebContents(); > > > + WebContents* web_contents = rfh_impl->delegate()->GetAsWebContents(); > > > + if (web_contents == nullptr) { > > > + InterstitialPageImpl* interstitial_page = > > > + static_cast<InterstitialPageImpl*>(rfh_impl->delegate()); > > > + if (interstitial_page->GetMainFrame() == rfh) { > > > + web_contents = interstitial_page->web_contents(); > > > + DCHECK(web_contents->ShowingInterstitialPage()); > > > + } > > > + } > > > + return web_contents; > > > } > > > > > > > > > That didn't work, and Nasko said he'd think about it. > > > > > > I'm happy to do something more "right", as long as it results in being able to > > > copy from interstitials soon. :-P > > > > Ah, that fix isn't safe. We can't assume the delegate is an > > InterstitialPageImpl just because GetAsWebContents returns null. > > > > I think it's worth giving the delegate approach a try, because then RWHVM will > > tell the correct object to do the copy operation for it. > > Also, Charlie and I already discussed several options, including the hack we attempted when I was in MTV. The conclusion was that Charlie's proposal was the cleanest of all. What should I do with this CL? I'm not sure of the scope of Charlie's suggestion.
> What should I do with this CL? I'm not sure of the scope of Charlie's > suggestion. Since Charlie is out until the 11th, talk with Nasko about how to do it Charlie's way. A VC meeting might be in order.
On 2015/05/06 21:42:49, Chris Palmer wrote: > > What should I do with this CL? I'm not sure of the scope of Charlie's > > suggestion. > > Since Charlie is out until the 11th, talk with Nasko about how to do it > Charlie's way. A VC meeting might be in order. Can you be a bit more specific? What "scope" are you unsure of? If you have a more precise question, let me know, otherwise a quick VC chat might be more useful to clarify what needs to be done.
On 2015/05/07 at 17:35:59, nasko wrote: > On 2015/05/06 21:42:49, Chris Palmer wrote: > > > What should I do with this CL? I'm not sure of the scope of Charlie's > > > suggestion. > > > > Since Charlie is out until the 11th, talk with Nasko about how to do it > > Charlie's way. A VC meeting might be in order. > > Can you be a bit more specific? What "scope" are you unsure of? > If you have a more precise question, let me know, otherwise a quick VC chat might be more useful to clarify what needs to be done. Scope as in "how much stuff would this touch?" I haven't yet had time to look at the code in order to get an idea of what would need to be done. I've put a meeting on your calnedar for Monday, if that works for you. :-)
On 2015/05/08 03:46:45, lgarron wrote: > On 2015/05/07 at 17:35:59, nasko wrote: > > On 2015/05/06 21:42:49, Chris Palmer wrote: > > > > What should I do with this CL? I'm not sure of the scope of Charlie's > > > > suggestion. > > > > > > Since Charlie is out until the 11th, talk with Nasko about how to do it > > > Charlie's way. A VC meeting might be in order. > > > > Can you be a bit more specific? What "scope" are you unsure of? > > If you have a more precise question, let me know, otherwise a quick VC chat > might be more useful to clarify what needs to be done. > > Scope as in "how much stuff would this touch?" > I haven't yet had time to look at the code in order to get an idea of what would > need to be done. I've put a meeting on your calnedar for Monday, if that works > for you. :-) I would expect that once you try out implementing it, you will get an idea of how much it touches. Let's meet after you've tried at least one attempt at implementing the suggestion. This way we can discuss concrete problems you might have hit or a meeting might not be needed.
On 2015/05/08 at 03:53:11, nasko wrote: > On 2015/05/08 03:46:45, lgarron wrote: > > On 2015/05/07 at 17:35:59, nasko wrote: > > > On 2015/05/06 21:42:49, Chris Palmer wrote: > > > > > What should I do with this CL? I'm not sure of the scope of Charlie's > > > > > suggestion. > > > > > > > > Since Charlie is out until the 11th, talk with Nasko about how to do it > > > > Charlie's way. A VC meeting might be in order. > > > > > > Can you be a bit more specific? What "scope" are you unsure of? > > > If you have a more precise question, let me know, otherwise a quick VC chat > > might be more useful to clarify what needs to be done. > > > > Scope as in "how much stuff would this touch?" > > I haven't yet had time to look at the code in order to get an idea of what would > > need to be done. I've put a meeting on your calnedar for Monday, if that works > > for you. :-) > > I would expect that once you try out implementing it, you will get an idea of how much it touches. Let's meet after you've tried at least one attempt at implementing the suggestion. This way we can discuss concrete problems you might have hit or a meeting might not be needed. Hmm. I'd be up for that, but I'm reluctant to start it if it might be big. I'll try to prioritize it among my smaller bugs.
On 2015/05/08 03:54:58, lgarron wrote: > On 2015/05/08 at 03:53:11, nasko wrote: > > On 2015/05/08 03:46:45, lgarron wrote: > > > On 2015/05/07 at 17:35:59, nasko wrote: > > > > On 2015/05/06 21:42:49, Chris Palmer wrote: > > > > > > What should I do with this CL? I'm not sure of the scope of Charlie's > > > > > > suggestion. > > > > > > > > > > Since Charlie is out until the 11th, talk with Nasko about how to do it > > > > > Charlie's way. A VC meeting might be in order. > > > > > > > > Can you be a bit more specific? What "scope" are you unsure of? > > > > If you have a more precise question, let me know, otherwise a quick VC > chat > > > might be more useful to clarify what needs to be done. > > > > > > Scope as in "how much stuff would this touch?" > > > I haven't yet had time to look at the code in order to get an idea of what > would > > > need to be done. I've put a meeting on your calnedar for Monday, if that > works > > > for you. :-) > > > > I would expect that once you try out implementing it, you will get an idea of > how much it touches. Let's meet after you've tried at least one attempt at > implementing the suggestion. This way we can discuss concrete problems you might > have hit or a meeting might not be needed. > > Hmm. I'd be up for that, but I'm reluctant to start it if it might be big. I'll > try to prioritize it among my smaller bugs. Hey Lucas, how about you take a look at the code and see if you understand the idea before scheduling a meeting? That way you know what kinds of questions you need to ask at the meeting. Don't spend a week on it but at least try it out a bit on your own first.
On 2015/05/08 at 14:16:24, felt wrote: > On 2015/05/08 03:54:58, lgarron wrote: > > On 2015/05/08 at 03:53:11, nasko wrote: > > > On 2015/05/08 03:46:45, lgarron wrote: > > > > On 2015/05/07 at 17:35:59, nasko wrote: > > > > > On 2015/05/06 21:42:49, Chris Palmer wrote: > > > > > > > What should I do with this CL? I'm not sure of the scope of Charlie's > > > > > > > suggestion. > > > > > > > > > > > > Since Charlie is out until the 11th, talk with Nasko about how to do it > > > > > > Charlie's way. A VC meeting might be in order. > > > > > > > > > > Can you be a bit more specific? What "scope" are you unsure of? > > > > > If you have a more precise question, let me know, otherwise a quick VC > > chat > > > > might be more useful to clarify what needs to be done. > > > > > > > > Scope as in "how much stuff would this touch?" > > > > I haven't yet had time to look at the code in order to get an idea of what > > would > > > > need to be done. I've put a meeting on your calnedar for Monday, if that > > works > > > > for you. :-) > > > > > > I would expect that once you try out implementing it, you will get an idea of > > how much it touches. Let's meet after you've tried at least one attempt at > > implementing the suggestion. This way we can discuss concrete problems you might > > have hit or a meeting might not be needed. > > > > Hmm. I'd be up for that, but I'm reluctant to start it if it might be big. I'll > > try to prioritize it among my smaller bugs. > > Hey Lucas, how about you take a look at the code and see if you understand the idea before scheduling a meeting? That way you know what kinds of questions you need to ask at the meeting. Don't spend a week on it but at least try it out a bit on your own first. I've made a new CL: https://codereview.chromium.org/1133003003 I'm closing this one, so that we can keep the discussion of the different implementations separate. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
