Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1398823004:
Switch the page-capturing machinery to use the new hooks. (Closed)
|
Created:
5 years, 2 months ago by dglazkov Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch the page-capturing machinery to use the new hooks.
This is a timing-changing patch. Here are the details:
* The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink.
* These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes.
* As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs.
* As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active.
* In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier.
* Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient.
* The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly.
BUG=521166
R=jam,andrewhayden,jochen
Committed: https://crrev.com/afc918de00a2f2895b4d8eaf3a043e654ba5e8fb
Cr-Commit-Position: refs/heads/master@{#372891}
Committed: https://crrev.com/50fcc022d021a25b746823002ad2b7e772649fb6
Cr-Commit-Position: refs/heads/master@{#373185}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase. #Patch Set 3 : How many explosions? #Patch Set 4 : Cleaned up. #Patch Set 5 : Caught one more test. #Patch Set 6 : Rebased. #Patch Set 7 : Removed merge oopsies. #Patch Set 8 : Worked some more on the tests. #Patch Set 9 : More test polishing. #Patch Set 10 : Removed another race in tests. #Patch Set 11 : Attempting to fix Linux/ChromeOS failures. #Patch Set 12 : Try to fix the bubble test. #Patch Set 13 : Maybe really all fixed now? #Patch Set 14 : Oic, this test was lookin' for love in all the wrong places. #Patch Set 15 : Imma dum-dum. #Patch Set 16 : Updated comments. #Patch Set 17 : Fixes http://crbug.com/583261. #
Messages
Total messages: 83 (34 generated)
dglazkov@chromium.org changed reviewers: + jam@chromium.org, jochen@chromium.org, toyoshim@chromium.org
https://codereview.chromium.org/1398823004/diff/1/chrome/browser/translate/tr... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/1398823004/diff/1/chrome/browser/translate/tr... chrome/browser/translate/translate_manager_browsertest.cc:76: Would love your insight here. After this change, the OnLanguageDetermined timing is no longer 500ms+ -- it's called as soon as the page content is ready. This exposes a fun race in this code here: before the change, the "About to set up... " log comes up before the "Language detected" log above. After this change, it's the other way around, which means that the WindowedNotificationObserver thing just sits there waiting forever -- the notification is issued before we're able to set up the observer. How do I deal with cases like this? Any ideas?
https://codereview.chromium.org/1398823004/diff/1/chrome/browser/translate/tr... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/1398823004/diff/1/chrome/browser/translate/tr... chrome/browser/translate/translate_manager_browsertest.cc:76: On 2015/10/09 at 23:22:22, dglazkov wrote: > Would love your insight here. After this change, the OnLanguageDetermined timing is no longer 500ms+ -- it's called as soon as the page content is ready. This exposes a fun race in this code here: before the change, the "About to set up... " log comes up before the "Language detected" log above. > > After this change, it's the other way around, which means that the WindowedNotificationObserver thing just sits there waiting forever -- the notification is issued before we're able to set up the observer. > > How do I deal with cases like this? Any ideas? I think the browser test harness is created before the session is restored, so if you create the notification observer as a member of TranslateManagerBrowserTest it should work
Rebase.
https://codereview.chromium.org/1398823004/diff/1/chrome/browser/translate/tr... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/1398823004/diff/1/chrome/browser/translate/tr... chrome/browser/translate/translate_manager_browsertest.cc:76: On 2015/10/12 at 11:23:50, jochen wrote: > I think the browser test harness is created before the session is restored, so if you create the notification observer as a member of TranslateManagerBrowserTest it should work The problem here is that the observer needs to have a source, and that's unknown until the InProcBrowserTests starts the loop, and it looks like the implementation there is pretty simplistic. For situations where the Webcontents already exists (I am guessing this includes session restore), it assumes that all important things will happen after load finished: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i... That's what makes the changed timing no longer work.
use content::NotificationService::AllSources() as source?
On 2015/10/20 at 09:51:32, jochen wrote: > use content::NotificationService::AllSources() as source? ZOMG, you just blew my mind.
The CQ bit was checked by dglazkov@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/1398823004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/40001
The CQ bit was checked by dglazkov@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/1398823004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dglazkov@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/1398823004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dglazkov@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/1398823004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Removed merge oopsies.
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Worked some more on the tests.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
More test polishing.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/160001
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Removed another race in tests.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Attempting to fix Linux/ChromeOS failures.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Try to fix the bubble test.
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Maybe really all fixed now?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Oic, this test was lookin' for love in all the wrong places.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/260001
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Imma dum-dum.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Switch the page-capturing machinery to use the new hooks. Currently WIP. ========== to ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help eliminate the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. ==========
dglazkov@chromium.org changed reviewers: + andrewhayden@chromium.org - toyoshim@chromium.org
Description was changed from ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help eliminate the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. ========== to ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. ==========
PTAL.
Description was changed from ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. ========== to ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. BUG=521166 R=jam,andrewhayden,jochen ==========
lgtm
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Updated comments.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dglazkov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1398823004/#ps300001 (title: "Updated comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. BUG=521166 R=jam,andrewhayden,jochen ========== to ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. BUG=521166 R=jam,andrewhayden,jochen Committed: https://crrev.com/afc918de00a2f2895b4d8eaf3a043e654ba5e8fb Cr-Commit-Position: refs/heads/master@{#372891} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/afc918de00a2f2895b4d8eaf3a043e654ba5e8fb Cr-Commit-Position: refs/heads/master@{#372891}
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1659393003/ by dglazkov@chromium.org. The reason for reverting is: Caused https://code.google.com/p/chromium/issues/detail?id=583261..
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Fixes http://crbug.com/583261.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dglazkov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1398823004/#ps320001 (title: "Fixes http://crbug.com/583261.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398823004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398823004/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. BUG=521166 R=jam,andrewhayden,jochen Committed: https://crrev.com/afc918de00a2f2895b4d8eaf3a043e654ba5e8fb Cr-Commit-Position: refs/heads/master@{#372891} ========== to ========== Switch the page-capturing machinery to use the new hooks. This is a timing-changing patch. Here are the details: * The page text capture machinery no longer uses archaic timers to determine the best time to grab the text from the the document. Instead, it relies on the MeaningfulLayout signals that are coming from Blink. * These signals are keyed off the compositor putting up frames, which has an interesting side effect -- at least on some platforms (CrOS and Linux), this signal will not be issued until document/frame/tab is active, which is perfectly fine for our purposes. * As a side comment, we should probably unify our position on whether compositor puts up frames for inactive tabs. * As another side comment, this would help simplify the logic in translate bubbles that watches for tab to become active. * In many cases, the timing is much shorter than before, especially for smaller pages. This is good (faster!), but also means that some tests had to be tweaked to start listening for language detection notifications a bit earlier. * Fundamentally, this is a change in timing, not behavior, so the existing test coverage seemed sufficient. * The new plumbing is still incompatible with out-of-process iframes (OOPIF) and will need to be fixed up accordingly. BUG=521166 R=jam,andrewhayden,jochen Committed: https://crrev.com/afc918de00a2f2895b4d8eaf3a043e654ba5e8fb Cr-Commit-Position: refs/heads/master@{#372891} Committed: https://crrev.com/50fcc022d021a25b746823002ad2b7e772649fb6 Cr-Commit-Position: refs/heads/master@{#373185} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/50fcc022d021a25b746823002ad2b7e772649fb6 Cr-Commit-Position: refs/heads/master@{#373185} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
