Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1162373002:
Make some editing/selection functions accessible to c/b/renderer_host/ (Closed)
|
Created:
5 years, 6 months ago by mohsen Modified:
5 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, mfomitchev, nasko+codewatch_chromium.org, lgarron Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake some editing/selection functions accessible to c/b/renderer_host/
In order to have access to Cut, Copy, Paste, MoveRangeSelectionExtent,
and SelectRange functions of WebConentsImp from c/b/renderer_host/,
specifically to implement unified touch selection for Aura, these
functions are added to RenderWidgetHostDelegate interface.
BUG=399721
Committed: https://crrev.com/7ab1ec16ecba562a54cd43ee6cef72bf3ef2de97
Cr-Commit-Position: refs/heads/master@{#337243}
Patch Set 1 #Patch Set 2 : Addressed sadrul@'s comments #
Total comments: 4
Patch Set 3 : Addressed nasko@'s comments #
Total comments: 2
Patch Set 4 : Implemented Cut/Copy/Paste for InterstitialPageImpl #Patch Set 5 : Fixed Mac compiler errors #Patch Set 6 : Used the new Cut/Copy/Paste in c/b/renderer_host/ #Patch Set 7 : Rebased #Patch Set 8 : Added some unit tests #
Total comments: 1
Patch Set 9 : Changed unit test to browser test #Patch Set 10 : Rebased #Patch Set 11 : --cq-dry-run #Patch Set 12 : Wait for page load #Patch Set 13 : Switched to oninput event as oncut/onpaste happen before cut/paste #
Total comments: 10
Patch Set 14 : Addressed review comments #
Total comments: 2
Patch Set 15 : Rebased (after editing commands were removed from ImeAdapterAndroid: crrev.com/336407) #
Total comments: 16
Patch Set 16 : Addressed review comments #
Total comments: 2
Patch Set 17 : Replaced an empty string with std::string() #Patch Set 18 : Added ClipboardMessageWatcher + Fixed Windows issues #Patch Set 19 : Simplified wait implementations #
Total comments: 13
Patch Set 20 : Addressed more review comments #
Total comments: 2
Patch Set 21 : ditto #
Total comments: 4
Patch Set 22 : Used scoped_refptr #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 61 (15 generated)
sadrul@: Can you please take a look at this patch before I send it to c/b/web_contnets/ owners?
mohsen@chromium.org changed reviewers: + sadrul@chromium.org
mohsen@chromium.org changed reviewers: + nasko@chromium.org
nasko@: Can you please take a look...
https://codereview.chromium.org/1162373002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1162373002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:83: virtual void Cut() {} Since these are also present in WebContents.h, I think they need to be pure virtual here as well. https://codereview.chromium.org/1162373002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1162373002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:567: // Following three functions are already listed under WebContents overrides. Why is this comment beneficial?
jam@chromium.org changed reviewers: + jam@chromium.org
can you change over the code in renderer_host that is calling out to webcontents over to these new methods?
nasko@: comments addressed, please take another look. jam@: the code in renderer_host/ that are calling these functions also call some other functions from WebContents. Is it fine to only change calls for these functions, or should I also add those other functions to the new interface in this patch? (Other functions are: Undo, Redo, CopyToFindPboard, PasteAndMatchStyle, SelectAll, Unselect, and places that call them are in ImeAdaptorAndroid and RenderWidgetHostViewMac) https://codereview.chromium.org/1162373002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1162373002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:83: virtual void Cut() {} On 2015/06/04 20:18:00, nasko wrote: > Since these are also present in WebContents.h, I think they need to be pure > virtual here as well. Yeah, event if they don't need to, making them pure virtual would make it cleaner/safer, though I need to add empty implementations to some derived classes. Done. https://codereview.chromium.org/1162373002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1162373002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:567: // Following three functions are already listed under WebContents overrides. On 2015/06/04 20:18:00, nasko wrote: > Why is this comment beneficial? If we don't include this comment and three below lines, someone reading the code might get confused at why these functions from RenderWidgetHostDelegate are not implemented. And, if we only include the following three commented lines, again it would be confusing as why these commented function declarations are in the code. So, I followed the similar pattern used in line 434.
https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:425: void InterstitialPageImpl::Cut() { I think these methods will need to actually have bodies, otherwise Cut/Copy/Paste on interstitial pages won't work. You can see some of the related work done in https://codereview.chromium.org/1133003003/
https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:425: void InterstitialPageImpl::Cut() { On 2015/06/05 00:14:38, nasko wrote: > I think these methods will need to actually have bodies, otherwise > Cut/Copy/Paste on interstitial pages won't work. You can see some of the related > work done in https://codereview.chromium.org/1133003003/ Done.
On 2015/06/05 02:02:10, mohsen wrote: > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > File content/browser/frame_host/interstitial_page_impl.cc (right): > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > content/browser/frame_host/interstitial_page_impl.cc:425: void > InterstitialPageImpl::Cut() { > On 2015/06/05 00:14:38, nasko wrote: > > I think these methods will need to actually have bodies, otherwise > > Cut/Copy/Paste on interstitial pages won't work. You can see some of the > related > > work done in https://codereview.chromium.org/1133003003/ > > Done. Did you address jam@'s comment? I don't see much changed in renderer_host. Also, what I'm concerned is whether we have enough test coverage for this move. For one, we don't know whether cut/copy/paste works on interstitials and we might inadvertently break it.
On 2015/06/05 15:45:19, nasko wrote: > On 2015/06/05 02:02:10, mohsen wrote: > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > File content/browser/frame_host/interstitial_page_impl.cc (right): > > > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > content/browser/frame_host/interstitial_page_impl.cc:425: void > > InterstitialPageImpl::Cut() { > > On 2015/06/05 00:14:38, nasko wrote: > > > I think these methods will need to actually have bodies, otherwise > > > Cut/Copy/Paste on interstitial pages won't work. You can see some of the > > related > > > work done in https://codereview.chromium.org/1133003003/ > > > > Done. > > Did you address jam@'s comment? I don't see much changed in renderer_host. Also, > what I'm concerned is whether we have enough test coverage for this move. For > one, we don't know whether cut/copy/paste works on interstitials and we might > inadvertently break it. I asked jam@ a question in comment #9 and I'm waiting for his response. In the meantime, I'll try to understand the situation with interstitials a bit more ...
On 2015/06/05 15:53:13, mohsen wrote: > On 2015/06/05 15:45:19, nasko wrote: > > On 2015/06/05 02:02:10, mohsen wrote: > > > > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > > File content/browser/frame_host/interstitial_page_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > > content/browser/frame_host/interstitial_page_impl.cc:425: void > > > InterstitialPageImpl::Cut() { > > > On 2015/06/05 00:14:38, nasko wrote: > > > > I think these methods will need to actually have bodies, otherwise > > > > Cut/Copy/Paste on interstitial pages won't work. You can see some of the > > > related > > > > work done in https://codereview.chromium.org/1133003003/ > > > > > > Done. > > > > Did you address jam@'s comment? I don't see much changed in renderer_host. > Also, > > what I'm concerned is whether we have enough test coverage for this move. For > > one, we don't know whether cut/copy/paste works on interstitials and we might > > inadvertently break it. > > I asked jam@ a question in comment #9 and I'm waiting for his response. In the > meantime, I'll try to understand the situation with interstitials a bit more ... at least for now, doing the cut/copy/paste so that we don't have duplicates should be done.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
On 2015/06/05 23:57:35, jam wrote: > On 2015/06/05 15:53:13, mohsen wrote: > > On 2015/06/05 15:45:19, nasko wrote: > > > On 2015/06/05 02:02:10, mohsen wrote: > > > > > > > > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > > > File content/browser/frame_host/interstitial_page_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > > > content/browser/frame_host/interstitial_page_impl.cc:425: void > > > > InterstitialPageImpl::Cut() { > > > > On 2015/06/05 00:14:38, nasko wrote: > > > > > I think these methods will need to actually have bodies, otherwise > > > > > Cut/Copy/Paste on interstitial pages won't work. You can see some of the > > > > related > > > > > work done in https://codereview.chromium.org/1133003003/ > > > > > > > > Done. > > > > > > Did you address jam@'s comment? I don't see much changed in renderer_host. > > Also, > > > what I'm concerned is whether we have enough test coverage for this move. > For > > > one, we don't know whether cut/copy/paste works on interstitials and we > might > > > inadvertently break it. > > > > I asked jam@ a question in comment #9 and I'm waiting for his response. In the > > meantime, I'll try to understand the situation with interstitials a bit more > ... > > at least for now, doing the cut/copy/paste so that we don't have duplicates > should be done. jam@: Done, for cut/copy/paste. nasko@: I have played a bit with some interstitials. Copy/Paste (and selection in general) seems to be already kind of broken on interstitials. For example, on SSL warning interstitial, if you select some text, you cannot drag it with mouse. Trying to drag with mouse starts a new selection as if there were no selection at all. Or, for the same SSL interstitial on Android, after selecting a word with long-press, pressing copy button from the action bar does nothing. Also, trying to drag handles, selection highlight and handles don't move, but Contextual Search window shows some text that are apparently coming from the previous page (or shows nothing if there is no previous page)! This issue probably deserves its own bug(s). What do you think?
On 2015/06/09 22:27:36, mohsen wrote: > On 2015/06/05 23:57:35, jam wrote: > > On 2015/06/05 15:53:13, mohsen wrote: > > > On 2015/06/05 15:45:19, nasko wrote: > > > > On 2015/06/05 02:02:10, mohsen wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > > > > File content/browser/frame_host/interstitial_page_impl.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1162373002/diff/40001/content/browser/frame_h... > > > > > content/browser/frame_host/interstitial_page_impl.cc:425: void > > > > > InterstitialPageImpl::Cut() { > > > > > On 2015/06/05 00:14:38, nasko wrote: > > > > > > I think these methods will need to actually have bodies, otherwise > > > > > > Cut/Copy/Paste on interstitial pages won't work. You can see some of > the > > > > > related > > > > > > work done in https://codereview.chromium.org/1133003003/ > > > > > > > > > > Done. > > > > > > > > Did you address jam@'s comment? I don't see much changed in renderer_host. > > > Also, > > > > what I'm concerned is whether we have enough test coverage for this move. > > For > > > > one, we don't know whether cut/copy/paste works on interstitials and we > > might > > > > inadvertently break it. > > > > > > I asked jam@ a question in comment #9 and I'm waiting for his response. In > the > > > meantime, I'll try to understand the situation with interstitials a bit more > > ... > > > > at least for now, doing the cut/copy/paste so that we don't have duplicates > > should be done. > > jam@: Done, for cut/copy/paste. > > nasko@: I have played a bit with some interstitials. Copy/Paste (and selection > in general) seems to be already kind of broken on interstitials. For example, on > SSL warning interstitial, if you select some text, you cannot drag it with > mouse. Trying to drag with mouse starts a new selection as if there were no > selection at all. Or, for the same SSL interstitial on Android, after selecting > a word with long-press, pressing copy button from the action bar does nothing. > Also, trying to drag handles, selection highlight and handles don't move, but > Contextual Search window shows some text that are apparently coming from the > previous page (or shows nothing if there is no previous page)! This issue > probably deserves its own bug(s). What do you think? I think interstitials do deserve their own bug, but at least copy/past should work on Aura platforms, right? Let's ensure we are at least not breaking those, though I think this CL will also make Copy/Paste to work on Mac. I wouldn't worry in this CL about all the other aspects (selection, drag, etc), but Copy is important as we display certificate details on interstitials and we want users to be able to copy them for bug reports.
Sorry for the delay! I was trying to understand interstitials better to write a test. I added a simple test to make sure cut/copy/paste IPC messages are sent properly for interstitials. Please take a look...
The CL looks great! Thanks for pushing through it! There is one concern I have. While the tests now verify that IPC is properly sent to the renderer, this is only half the work. Ensuring that the IPC coming back from the renderer goes to the right RWH delegate will be great to verify. This could be done through unit test (faking and calling OnMessageReceived on the right object) or writing a browser test. https://codereview.chromium.org/1162373002/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1162373002/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl_unittest.cc:2470: EXPECT_TRUE(sink.GetUniqueMessageMatching(InputMsg_Cut::ID)); Let's also verify that the routing id in the message is the expected one (routing id of the focused frame.)
On 2015/06/15 21:59:45, nasko (very slow . paris) wrote: > The CL looks great! Thanks for pushing through it! > > There is one concern I have. While the tests now verify that IPC is properly > sent to the renderer, this is only half the work. Ensuring that the IPC coming > back from the renderer goes to the right RWH delegate will be great to verify. > This could be done through unit test (faking and calling OnMessageReceived on > the right object) or writing a browser test. I'm not sure I understand what you want here correctly. By the IPCs coming back from the renderer, you mean ClipboardHostMsg_* IPCs? They seem to be handled by ClipboardMessageFilter and no RWH delegate is involved. Maybe, we can add a browser test that performs copy/cut/paste on an interstitial page and checks the clipboard/content of the page to make sure it was performed successfully? > https://codereview.chromium.org/1162373002/diff/200001/content/browser/web_co... > File content/browser/web_contents/web_contents_impl_unittest.cc (right): > > https://codereview.chromium.org/1162373002/diff/200001/content/browser/web_co... > content/browser/web_contents/web_contents_impl_unittest.cc:2470: > EXPECT_TRUE(sink.GetUniqueMessageMatching(InputMsg_Cut::ID)); > Let's also verify that the routing id in the message is the expected one > (routing id of the focused frame.)
On 2015/06/16 23:24:22, mohsen wrote: > On 2015/06/15 21:59:45, nasko (very slow . paris) wrote: > > The CL looks great! Thanks for pushing through it! > > > > There is one concern I have. While the tests now verify that IPC is properly > > sent to the renderer, this is only half the work. Ensuring that the IPC coming > > back from the renderer goes to the right RWH delegate will be great to verify. > > This could be done through unit test (faking and calling OnMessageReceived on > > the right object) or writing a browser test. > > I'm not sure I understand what you want here correctly. By the IPCs coming back > from the renderer, you mean ClipboardHostMsg_* IPCs? They seem to be handled by > ClipboardMessageFilter and no RWH delegate is involved. When I looked at this last time with lgarron@, the copy didn't work on interstitials, even though the Copy IPC was sent from the browser to the correct renderer and RenderFrameImpl. There was some issue on the IPC reply, but I don't recall the details. > Maybe, we can add a > browser test that performs copy/cut/paste on an interstitial page and checks the > clipboard/content of the page to make sure it was performed successfully? This is indeed what I had in mind. > https://codereview.chromium.org/1162373002/diff/200001/content/browser/web_co... > > File content/browser/web_contents/web_contents_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/1162373002/diff/200001/content/browser/web_co... > > content/browser/web_contents/web_contents_impl_unittest.cc:2470: > > EXPECT_TRUE(sink.GetUniqueMessageMatching(InputMsg_Cut::ID)); > > Let's also verify that the routing id in the message is the expected one > > (routing id of the focused frame.)
Patchset #9 (id:220001) has been deleted
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162373002/270001
nasko@: I've changed the test to a browser test that performs cut/copy/paste and at the end checks the (mock) clipboard (patch set 9). Can you take a look and let me know what you think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
nasko@chromium.org changed reviewers: + dcheng@chromium.org
This is really awesome! Thanks for taking the time to do it right! It does seem that the paste test is failing on Windows, so this needs to be understood and fixed before committing this. I'm adding dcheng@, who is our clipboard expert, for heads up and/or ideas of why Windows is failing. https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:50: class TitleUpdateWatcher : public BrowserMessageFilter { Is the existing TitleWatcher insufficient for this use case? https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:152: clipboard_->ResetWaits(); Why not use the title watcher here as well? Mostly for consistency and avoiding others asking you the same question :). https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:174: &clipboard_text); nit: This indentation doesn't look right. Should be aligned with "ui::...". https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:189: // Create the interstitial page. This is a lot of setup that is happening before the test case starts. I'm worried that certain state might change between here and when the test case starts. Why not create a "GetReadyForTest" type method and call it at the beginning of each case? You already do that with ClearClipboard, which you can fold into this new method.
Other than addressing comments, I've also changed TestClipboard::CreateForCurrentThread() to return a TestClipboard* instead of Clipboard* to avoid unnecessary casts. And, destroyed clipboard at the end. The windows failures look weird. It seems that it pastes some random text into the textfield (from the real clipboard? it shouldn't happen as I'm using the TestClipboard). Also, in win_chromium_x64_rel_ng bot, TextfieldModelTest.Clipboard (which is not related to my change at all) is failed once and then passed. dcheng@: What do you think? https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:50: class TitleUpdateWatcher : public BrowserMessageFilter { On 2015/06/26 11:35:39, nasko (paris) wrote: > Is the existing TitleWatcher insufficient for this use case? > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... The existing one does not work for interstitial pages. Maybe we can extend it to include interstitial pages, too. But, that seemed more work which I'm not sure would be useful in future or not? https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:152: clipboard_->ResetWaits(); On 2015/06/26 11:35:38, nasko (paris) wrote: > Why not use the title watcher here as well? Mostly for consistency and avoiding > others asking you the same question :). The title is only updated on events that change value of the textfield. Copying does not change it. https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:174: &clipboard_text); On 2015/06/26 11:35:38, nasko (paris) wrote: > nit: This indentation doesn't look right. Should be aligned with "ui::...". Done. https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:189: // Create the interstitial page. On 2015/06/26 11:35:38, nasko (paris) wrote: > This is a lot of setup that is happening before the test case starts. I'm > worried that certain state might change between here and when the test case > starts. Why not create a "GetReadyForTest" type method and call it at the > beginning of each case? You already do that with ClearClipboard, which you can > fold into this new method. Done.
Patchset #15 (id:350001) has been deleted
I think it looks good, so LGTM from me. I'd still like to see what dcheng@ has to say about the clipboard bits though. https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:50: class TitleUpdateWatcher : public BrowserMessageFilter { On 2015/06/26 19:57:56, mohsen wrote: > On 2015/06/26 11:35:39, nasko (paris) wrote: > > Is the existing TitleWatcher insufficient for this use case? > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > The existing one does not work for interstitial pages. Maybe we can extend it to > include interstitial pages, too. But, that seemed more work which I'm not sure > would be useful in future or not? If this is the case, let's name the class to indicate it works for interstitials. Also, a comment describing why we need it is warranted, as anyone seeing it, will likely question the duplication just as I did. https://codereview.chromium.org/1162373002/diff/330001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/330001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:124: web_contents_ = static_cast<WebContentsImpl*>(shell()->web_contents()); Why not just use the shell()->web_contents()? Having an extra local variable doesn't seem very useful, unless we change WebContents during the test, which doesn't seem to be the case.
The test is failing on Windows because writing to the clipboard occurs on the IO thread but only the UI thread had a test clipboard injected. The "foobar" value is coming from one of the extension tests: I've filed a bug about this test target, since it should either use the test clipboard or be moved into interactive_ui_tests. I will say I'm not wildly enthusiastic about adding the waits to the test clipboard. I'd almost like to say it should be a separate ClipboardWatcher class like TitleWatcher. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:434: if (!focused_frame) Question: can you have a situation where focused_node != nullptr and focused_frame == nullptr? If not, I think it'd be clearer to rewrite this as: FrameTreeNode* focused_node = ...; if (!focused_node) return; RenderFrameHostImpl* focused_frame = focused_node->current_frame_host(); https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:457: return; It almost seems like we should have a helper for this. How common will this pattern be? https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:50: class TitleUpdateWatcher : public BrowserMessageFilter { How come we can't just use TitleWatcher? https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:63: void Wait() { You should probably just combine this with InitWait(). Is there a reason to separate them? https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:163: ExecuteScriptAndGetValue(interstitial_->GetMainFrame(), The comment on this function is: // This should not be used; the use of the ExecuteScript functions in // browser_test_utils is preferable. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:171: input_value->GetAsString(&input_text); GetAsString() can fail.
Regarding Windows issue: what can we do to fix that? Can we share the TestClipboard between UI and IO threads? Regarding waits in TestClipboard: creating a separate ClipboardWatcher would probably also need some sort of ClipboardObserver. I'm not sure that would be useful out of this test and even in that case it probably needs its own CL. What do you think? https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/310001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:50: class TitleUpdateWatcher : public BrowserMessageFilter { On 2015/06/29 09:30:24, nasko (paris) wrote: > On 2015/06/26 19:57:56, mohsen wrote: > > On 2015/06/26 11:35:39, nasko (paris) wrote: > > > Is the existing TitleWatcher insufficient for this use case? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > > > The existing one does not work for interstitial pages. Maybe we can extend it > to > > include interstitial pages, too. But, that seemed more work which I'm not sure > > would be useful in future or not? > > If this is the case, let's name the class to indicate it works for > interstitials. Also, a comment describing why we need it is warranted, as anyone > seeing it, will likely question the duplication just as I did. Done. https://codereview.chromium.org/1162373002/diff/330001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/330001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:124: web_contents_ = static_cast<WebContentsImpl*>(shell()->web_contents()); On 2015/06/29 09:30:24, nasko (paris) wrote: > Why not just use the shell()->web_contents()? Having an extra local variable > doesn't seem very useful, unless we change WebContents during the test, which > doesn't seem to be the case. Right. That was needed in a previous version of my CL to avoid casting to WebContentsImpl* every time. Not needed anymore. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:434: if (!focused_frame) On 2015/06/29 17:58:02, dcheng wrote: > Question: can you have a situation where focused_node != nullptr and > focused_frame == nullptr? > > If not, I think it'd be clearer to rewrite this as: > FrameTreeNode* focused_node = ...; > if (!focused_node) return; > RenderFrameHostImpl* focused_frame = focused_node->current_frame_host(); This is the case I saw in tests and that's why I explicitly set focused frame in my interstitial browser test. However, it appears to me that it should not be the case (based on other places that FrameTreeNode::current_frame_host() is used). So, changed it to your suggestion. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:457: return; On 2015/06/29 17:58:02, dcheng wrote: > It almost seems like we should have a helper for this. How common will this > pattern be? Not applicable, anymore, I guess. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:50: class TitleUpdateWatcher : public BrowserMessageFilter { On 2015/06/29 17:58:02, dcheng wrote: > How come we can't just use TitleWatcher? TitleWatcher is not working for interstitial pages. Per nasko@'s suggestion, I've renamed this class and added comments to make it clear. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:63: void Wait() { On 2015/06/29 17:58:02, dcheng wrote: > You should probably just combine this with InitWait(). Is there a reason to > separate them? The problem is that the message we are waiting for might arrive just before Wait() is called in which case we would wait indefinitely. InitWait() is called when we're sure the message has not yet arrived. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:163: ExecuteScriptAndGetValue(interstitial_->GetMainFrame(), On 2015/06/29 17:58:02, dcheng wrote: > The comment on this function is: > // This should not be used; the use of the ExecuteScript functions in > // browser_test_utils is preferable. Done. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:171: input_value->GetAsString(&input_text); On 2015/06/29 17:58:02, dcheng wrote: > GetAsString() can fail. Not applicable, anymore.
On 2015/06/29 at 21:46:54, mohsen wrote: > Regarding Windows issue: what can we do to fix that? Can we share the TestClipboard between UI and IO threads? > > Regarding waits in TestClipboard: creating a separate ClipboardWatcher would probably also need some sort of ClipboardObserver. I'm not sure that would be useful out of this test and even in that case it probably needs its own CL. What do you think? You could post a task to call TestClipboard::CreateFromClipboard() on the IO thread. I object to adding a waiter interface to TestClipboard because all Clipboard methods are synchronous: by definition, calling a method on them synchronously does something. The fact that clipboard operations happen asynchronously is an artifact of the ClipboardMessageFilter implementation. I think it'd probably make more sense to add a BrowserMessageFilter that intercepts clipboard messages, like what you're already doing for title changes. https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:63: void Wait() { On 2015/06/29 at 21:46:53, mohsen wrote: > On 2015/06/29 17:58:02, dcheng wrote: > > You should probably just combine this with InitWait(). Is there a reason to > > separate them? > > The problem is that the message we are waiting for might arrive just before Wait() is called in which case we would wait indefinitely. InitWait() is called when we're sure the message has not yet arrived. You can call RunLoop::Quit before calling RunLoop::Run. This can be simplified to: class TitleUpdateWatcher : public BrowserMessageFilter { public: explicit TitleUpdateWatcher(const std::string& expected) : BrowserMessageFilter(FrameMsgStart), expected_(expected) { } void Wait() { run_loop_.Run(); } private: void OnTitleChanged(const std::string& title) { if (expected_ == title) run_loop_.Quit(); } bool OnMessageReceived(...) override { /* ... */ } base::RunLoop run_loop_; const std::string expected_; }; https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:141: title_update_watcher_ = new TitleUpdateWatcher; Incidentally, this is leaking atm, but with the suggested changes, you won't want to heap allocate this anyway. https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:263: ASSERT_TRUE(SetInputText("")); Nit: std::string()
On 2015/06/29 22:02:56, dcheng wrote: > On 2015/06/29 at 21:46:54, mohsen wrote: > > Regarding Windows issue: what can we do to fix that? Can we share the > TestClipboard between UI and IO threads? > > > > Regarding waits in TestClipboard: creating a separate ClipboardWatcher would > probably also need some sort of ClipboardObserver. I'm not sure that would be > useful out of this test and even in that case it probably needs its own CL. What > do you think? > > You could post a task to call TestClipboard::CreateFromClipboard() on the IO > thread. Do you mean TestClipboard::CreateForCurrentThread()? If yes, does this mean that I should have two instances of TestClipboard; one for each thread? > I object to adding a waiter interface to TestClipboard because all Clipboard > methods are synchronous: by definition, calling a method on them synchronously > does something. The fact that clipboard operations happen asynchronously is an > artifact of the ClipboardMessageFilter implementation. > > I think it'd probably make more sense to add a BrowserMessageFilter that > intercepts clipboard messages, like what you're already doing for title changes. An issue with this approach is that ClipboardMessageFilter is a BrowserMessageFilter itself and if it gets the clipboard message before my filter, it would mark it as handled and my filter would not see the message at all. And, I'm not sure if I can install my filter before the ClipboardMessageFilter. > https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... > File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): > > https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... > content/browser/frame_host/interstitial_page_impl_browsertest.cc:63: void Wait() > { > On 2015/06/29 at 21:46:53, mohsen wrote: > > On 2015/06/29 17:58:02, dcheng wrote: > > > You should probably just combine this with InitWait(). Is there a reason to > > > separate them? > > > > The problem is that the message we are waiting for might arrive just before > Wait() is called in which case we would wait indefinitely. InitWait() is called > when we're sure the message has not yet arrived. > > You can call RunLoop::Quit before calling RunLoop::Run. > > This can be simplified to: > class TitleUpdateWatcher : public BrowserMessageFilter { > public: > explicit TitleUpdateWatcher(const std::string& expected) > : BrowserMessageFilter(FrameMsgStart), > expected_(expected) { } > > void Wait() { > run_loop_.Run(); > } > > private: > void OnTitleChanged(const std::string& title) { > if (expected_ == title) > run_loop_.Quit(); > } > > bool OnMessageReceived(...) override { /* ... */ } > > base::RunLoop run_loop_; > const std::string expected_; > > }; > > https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... > content/browser/frame_host/interstitial_page_impl_browsertest.cc:141: > title_update_watcher_ = new TitleUpdateWatcher; > Incidentally, this is leaking atm, but with the suggested changes, you won't > want to heap allocate this anyway. > > https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... > File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): > > https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... > content/browser/frame_host/interstitial_page_impl_browsertest.cc:263: > ASSERT_TRUE(SetInputText("")); > Nit: std::string()
https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:63: void Wait() { On 2015/06/29 22:02:56, dcheng wrote: > On 2015/06/29 at 21:46:53, mohsen wrote: > > On 2015/06/29 17:58:02, dcheng wrote: > > > You should probably just combine this with InitWait(). Is there a reason to > > > separate them? > > > > The problem is that the message we are waiting for might arrive just before > Wait() is called in which case we would wait indefinitely. InitWait() is called > when we're sure the message has not yet arrived. > > You can call RunLoop::Quit before calling RunLoop::Run. > > This can be simplified to: > class TitleUpdateWatcher : public BrowserMessageFilter { > public: > explicit TitleUpdateWatcher(const std::string& expected) > : BrowserMessageFilter(FrameMsgStart), > expected_(expected) { } > > void Wait() { > run_loop_.Run(); > } > > private: > void OnTitleChanged(const std::string& title) { > if (expected_ == title) > run_loop_.Quit(); > } > > bool OnMessageReceived(...) override { /* ... */ } > > base::RunLoop run_loop_; > const std::string expected_; > > }; If this is meant to be allocated on stack, I think it's not possible as BrowserMessageFilter's are ref-counted and have private destructors. https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:263: ASSERT_TRUE(SetInputText("")); On 2015/06/29 22:02:56, dcheng wrote: > Nit: std::string() Done.
On 2015/06/29 at 22:53:59, mohsen wrote: > On 2015/06/29 22:02:56, dcheng wrote: > > On 2015/06/29 at 21:46:54, mohsen wrote: > > > Regarding Windows issue: what can we do to fix that? Can we share the > > TestClipboard between UI and IO threads? > > > > > > Regarding waits in TestClipboard: creating a separate ClipboardWatcher would > > probably also need some sort of ClipboardObserver. I'm not sure that would be > > useful out of this test and even in that case it probably needs its own CL. What > > do you think? > > > > You could post a task to call TestClipboard::CreateFromClipboard() on the IO > > thread. > Do you mean TestClipboard::CreateForCurrentThread()? If yes, does this mean that I should have two instances of TestClipboard; one for each thread? Yes, that's the implication. > > > I object to adding a waiter interface to TestClipboard because all Clipboard > > methods are synchronous: by definition, calling a method on them synchronously > > does something. The fact that clipboard operations happen asynchronously is an > > artifact of the ClipboardMessageFilter implementation. > > > > I think it'd probably make more sense to add a BrowserMessageFilter that > > intercepts clipboard messages, like what you're already doing for title changes. > An issue with this approach is that ClipboardMessageFilter is a BrowserMessageFilter itself and if it gets the clipboard message before my filter, it would mark it as handled and my filter would not see the message at all. And, I'm not sure if I can install my filter before the ClipboardMessageFilter. I'm going to cheat a little here. From MessageFilterRouter: // List of global and selective filters; a given filter will exist in either // |message_global_filters_| OR |message_class_filters_|, but not both. // Note that |message_global_filters_| will be given first offering of any // given message. It's the filter implementer and installer's // responsibility to ensure that a filter is either global or selective to // ensure proper message filtering order. So your filter would be a global message filter, which guarantees that it runs before the real ClipboardMessageFilter. > > > https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... > > File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): > > > > https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... > > content/browser/frame_host/interstitial_page_impl_browsertest.cc:63: void Wait() > > { > > On 2015/06/29 at 21:46:53, mohsen wrote: > > > On 2015/06/29 17:58:02, dcheng wrote: > > > > You should probably just combine this with InitWait(). Is there a reason to > > > > separate them? > > > > > > The problem is that the message we are waiting for might arrive just before > > Wait() is called in which case we would wait indefinitely. InitWait() is called > > when we're sure the message has not yet arrived. > > > > You can call RunLoop::Quit before calling RunLoop::Run. > > > > This can be simplified to: > > class TitleUpdateWatcher : public BrowserMessageFilter { > > public: > > explicit TitleUpdateWatcher(const std::string& expected) > > : BrowserMessageFilter(FrameMsgStart), > > expected_(expected) { } > > > > void Wait() { > > run_loop_.Run(); > > } > > > > private: > > void OnTitleChanged(const std::string& title) { > > if (expected_ == title) > > run_loop_.Quit(); > > } > > > > bool OnMessageReceived(...) override { /* ... */ } > > > > base::RunLoop run_loop_; > > const std::string expected_; > > > > }; > > > > https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... > > content/browser/frame_host/interstitial_page_impl_browsertest.cc:141: > > title_update_watcher_ = new TitleUpdateWatcher; > > Incidentally, this is leaking atm, but with the suggested changes, you won't > > want to heap allocate this anyway. > > > > https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... > > File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): > > > > https://codereview.chromium.org/1162373002/diff/390001/content/browser/frame_... > > content/browser/frame_host/interstitial_page_impl_browsertest.cc:263: > > ASSERT_TRUE(SetInputText("")); > > Nit: std::string()
https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/370001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:63: void Wait() { On 2015/06/29 at 22:56:29, mohsen wrote: > On 2015/06/29 22:02:56, dcheng wrote: > > On 2015/06/29 at 21:46:53, mohsen wrote: > > > On 2015/06/29 17:58:02, dcheng wrote: > > > > You should probably just combine this with InitWait(). Is there a reason to > > > > separate them? > > > > > > The problem is that the message we are waiting for might arrive just before > > Wait() is called in which case we would wait indefinitely. InitWait() is called > > when we're sure the message has not yet arrived. > > > > You can call RunLoop::Quit before calling RunLoop::Run. > > > > This can be simplified to: > > class TitleUpdateWatcher : public BrowserMessageFilter { > > public: > > explicit TitleUpdateWatcher(const std::string& expected) > > : BrowserMessageFilter(FrameMsgStart), > > expected_(expected) { } > > > > void Wait() { > > run_loop_.Run(); > > } > > > > private: > > void OnTitleChanged(const std::string& title) { > > if (expected_ == title) > > run_loop_.Quit(); > > } > > > > bool OnMessageReceived(...) override { /* ... */ } > > > > base::RunLoop run_loop_; > > const std::string expected_; > > > > }; > > If this is meant to be allocated on stack, I think it's not possible as BrowserMessageFilter's are ref-counted and have private destructors. You can still simplify the internal state management even if that's the case. Just hold a RunLoop in a scoped_ptr and reset it on InitWait(). This gets rid of two state variables, and removes the need to use base::AutoReset.
dcheng@: Please take another look. I've changed to following: - Removed wait logic from TestClipboard and instead created a ClipboardMessageWatcher to wait on IPC messages. Therefore, I don't need a TestClipboard at all for cut and copy. - Created a TestClipboard for paste. It is created on the IO thread for Windows and on the UI thread for other platforms. - Simplified wait logic as you suggested.
https://codereview.chromium.org/1162373002/diff/450001/content/common/clipboa... File content/common/clipboard_messages.h (right): https://codereview.chromium.org/1162373002/diff/450001/content/common/clipboa... content/common/clipboard_messages.h:31: ??
https://codereview.chromium.org/1162373002/diff/450001/content/common/clipboa... File content/common/clipboard_messages.h (right): https://codereview.chromium.org/1162373002/diff/450001/content/common/clipboa... content/common/clipboard_messages.h:31: On 2015/06/30 22:01:18, sadrul wrote: > ?? There was a link error for ClipboardHostMsg_WriteText::Read(). I copied this from frame_messages.h and it fixed the link error.
https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:7: #include "base/auto_reset.h" This is no longer needed. https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:67: expected_title_ = base::ASCIIToUTF16(expected_title); Nit: UTF8ToUTF16 is probably a bit more future proof. https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:243: new InterstitialTitleUpdateWatcher(interstitial_.get()); These are leaked right now. Will the bots complain? https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:328: base::string16 clipboard_text = PerformCut(); I'd recommend having the helpers convert to std::string, so the expectations read more naturally.
https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:7: #include "base/auto_reset.h" On 2015/07/02 07:06:25, dcheng wrote: > This is no longer needed. Thanks for catching this. Removed. https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:67: expected_title_ = base::ASCIIToUTF16(expected_title); On 2015/07/02 07:06:25, dcheng wrote: > Nit: UTF8ToUTF16 is probably a bit more future proof. Done. https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:243: new InterstitialTitleUpdateWatcher(interstitial_.get()); On 2015/07/02 07:06:25, dcheng wrote: > These are leaked right now. Will the bots complain? My understanding is that these are ref-counted and will be deleted when they are removed from the message filter list at shutdown time. Isn't it right? https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:328: base::string16 clipboard_text = PerformCut(); On 2015/07/02 07:06:25, dcheng wrote: > I'd recommend having the helpers convert to std::string, so the expectations > read more naturally. Done.
https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:243: new InterstitialTitleUpdateWatcher(interstitial_.get()); On 2015/07/02 at 16:41:01, mohsen wrote: > On 2015/07/02 07:06:25, dcheng wrote: > > These are leaked right now. Will the bots complain? > > My understanding is that these are ref-counted and will be deleted when they are removed from the message filter list at shutdown time. Isn't it right? I suppose that's right, but it wasn't obvious to me at first glance. https://codereview.chromium.org/1162373002/diff/470001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/470001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:280: return base::UTF16ToUTF8(clipboard_message_watcher_->last_text()); Just save last_text() as a std::string and convert it when you receive it in the message filter.
https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:243: new InterstitialTitleUpdateWatcher(interstitial_.get()); On 2015/07/02 16:53:28, dcheng wrote: > On 2015/07/02 at 16:41:01, mohsen wrote: > > On 2015/07/02 07:06:25, dcheng wrote: > > > These are leaked right now. Will the bots complain? > > > > My understanding is that these are ref-counted and will be deleted when they > are removed from the message filter list at shutdown time. Isn't it right? > > I suppose that's right, but it wasn't obvious to me at first glance. Commented this in code. https://codereview.chromium.org/1162373002/diff/470001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/470001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:280: return base::UTF16ToUTF8(clipboard_message_watcher_->last_text()); On 2015/07/02 16:53:29, dcheng wrote: > Just save last_text() as a std::string and convert it when you receive it in the > message filter. Done.
https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/450001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:243: new InterstitialTitleUpdateWatcher(interstitial_.get()); On 2015/07/02 17:03:45, mohsen wrote: > On 2015/07/02 16:53:28, dcheng wrote: > > On 2015/07/02 at 16:41:01, mohsen wrote: > > > On 2015/07/02 07:06:25, dcheng wrote: > > > > These are leaked right now. Will the bots complain? > > > > > > My understanding is that these are ref-counted and will be deleted when they > > are removed from the message filter list at shutdown time. Isn't it right? > > > > I suppose that's right, but it wasn't obvious to me at first glance. > > Commented this in code. I mean documented...
LGTM with nits. https://codereview.chromium.org/1162373002/diff/490001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/490001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:175: : clipboard_message_watcher_(nullptr), title_update_watcher_(nullptr) {} You won't need these initializers either with the below suggestion. https://codereview.chromium.org/1162373002/diff/490001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:240: // when they are removed from the filter message list at shutdown time. IMO, just make these members scoped_refptr and avoid any questions about it.
https://codereview.chromium.org/1162373002/diff/490001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/1162373002/diff/490001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:175: : clipboard_message_watcher_(nullptr), title_update_watcher_(nullptr) {} On 2015/07/02 17:09:54, dcheng wrote: > You won't need these initializers either with the below suggestion. Removed. https://codereview.chromium.org/1162373002/diff/490001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:240: // when they are removed from the filter message list at shutdown time. On 2015/07/02 17:09:54, dcheng wrote: > IMO, just make these members scoped_refptr and avoid any questions about it. Yeah, that's much better. Done.
The CQ bit was checked by mohsen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1162373002/#ps510001 (title: "Used scoped_refptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162373002/510001
Message was sent while issue was closed.
Committed patchset #22 (id:510001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/7ab1ec16ecba562a54cd43ee6cef72bf3ef2de97 Cr-Commit-Position: refs/heads/master@{#337243}
Message was sent while issue was closed.
lgarron@chromium.org changed reviewers: + lgarron@chromium.org
Message was sent while issue was closed.
I had a similar CL at https://codereview.chromium.org/1133003003 It was going to move all of the following methods to the RWHD: void Undo() override; void Redo() override; void Cut() override; void Copy() override; void CopyToFindPboard() override; void Paste() override; void PasteAndMatchStyle() override; void SelectAll() override; Were there particular reasons for only doing Cut/Copy/Paste in this CL?
Message was sent while issue was closed.
lgarron@chromium.org changed reviewers: - lgarron@chromium.org
Message was sent while issue was closed.
On 2015/07/06 21:00:12, lgarron wrote: > I had a similar CL at https://codereview.chromium.org/1133003003 > > It was going to move all of the following methods to the RWHD: > void Undo() override; > void Redo() override; > void Cut() override; > void Copy() override; > void CopyToFindPboard() override; > void Paste() override; > void PasteAndMatchStyle() override; > void SelectAll() override; > > Were there particular reasons for only doing Cut/Copy/Paste in this CL? lgarron@: Cut/Copy/Paste was what I needed to unblock my touch selection unification patch (http://crrev.com/698253004). The rest should probably land as a separate CL and then the Mac bug (http://crbug.com/415784) should be marked as fixed?
Message was sent while issue was closed.
On 2015/07/06 at 21:27:47, mohsen wrote: > On 2015/07/06 21:00:12, lgarron wrote: > > I had a similar CL at https://codereview.chromium.org/1133003003 > > > > It was going to move all of the following methods to the RWHD: > > void Undo() override; > > void Redo() override; > > void Cut() override; > > void Copy() override; > > void CopyToFindPboard() override; > > void Paste() override; > > void PasteAndMatchStyle() override; > > void SelectAll() override; > > > > Were there particular reasons for only doing Cut/Copy/Paste in this CL? > > lgarron@: Cut/Copy/Paste was what I needed to unblock my touch selection unification patch (http://crrev.com/698253004). The rest should probably land as a separate CL and then the Mac bug (http://crbug.com/415784) should be marked as fixed? mohsen@: Thanks for the explanation. I've created a CL for SelectAll: https://codereview.chromium.org/1212373009 I'd like to hear from creis@ or nasko@ about whether to do the others. |
