Skip a first-run import on Mac when there is nothing left to import.

Skip a first-run import on Mac when there is nothing left to import.

This can happen if (e.g.) master_preferences filters out all things 
available to import (e.g.) from Safari. The import process on mac still 
starts, but has nothing to import. This causes it to hang, because the 
Import_Finished IPC is hooked onto the last item (not on NotifyEnded).

This change also adds a regression test.

BUG=169984
TEST=Added browser_test:FirstRunMasterPrefsBrowserTest.ImportNothingAndShowNewTabPage

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180039

[tapted, 2013-01-30 11:25:57]

cpu: can you please take a look for review and OWNERS.

The fix itself is just a couple of lines. All the gory details are at
http://crbug.com/169984#c16

The rest of the change is a regression test. (Issues in the first-run
integration have come up a few times recently, often close to release..)

Thanks!

[cpu_(ooo_6.6-7.5), 2013-01-30 21:00:53]

https://codereview.chromium.org/12094026/diff/1002/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/12094026/diff/1002/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_browsertest.cc:96:
ASSERT_TRUE(file_util::CreateTemporaryFile(&prefs_file));
who deletes this temporary file?

https://codereview.chromium.org/12094026/diff/1002/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_browsertest.cc:135: // is only added after
the import process is complete.
this test seems very fagile. There must be a better way to the detect the NTP?

[tapted, 2013-01-31 00:47:16]

https://codereview.chromium.org/12094026/diff/1002/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/12094026/diff/1002/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_browsertest.cc:96:
ASSERT_TRUE(file_util::CreateTemporaryFile(&prefs_file));
On 2013/01/30 21:00:53, cpu wrote:
> who deletes this temporary file?

Fixed (how did I miss that..).

https://codereview.chromium.org/12094026/diff/1002/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_browsertest.cc:135: // is only added after
the import process is complete.
On 2013/01/30 21:00:53, cpu wrote:
> this test seems very fagile. There must be a better way to the detect the NTP?

This last step is more to address the specific observation from the bug report,
where the NTP loaded OK, but was missing the webstore app icon. The tests in
new_tab_ui_browsertest.cc stop after the check on GetMaxPageID (e.g. [1]).

ui_test_utils::FindInPage is used, e.g., in tests for about://version [2]. But
you are right that if the NTP changes the occurrences of 'Web Store', then this
test would need to be updated.

One alternative might be to convert the test to a WebUIBrowserTest, along the
lines of most_visited_page_test.js [3]. This might be able to check specifically
for the number of app icons displayed on the NTP (or just that one of them is
WebStore, in case more are added). Shall I pursue this?

Or, perhaps the last step is not that important and can be removed. Since, the
root-cause of the bug ended up being the import process hanging, and the code in
the SetUp is enough to cause this test to timeout without the fix.

Let me know what you think. 


[1]
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/ui/we...
[2]
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/ui/br...
[3]
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/ui/we...

[cpu_(ooo_6.6-7.5), 2013-01-31 01:19:20]

You seem to know more than me about these kind of tests, I personally prefer
simple and reliable which might be GetMaxPageID

So lets do that unless you feel strongly about other option. I do think is
valuable to have the signal that the NTP or some other signal that chrome is
healthy is good.

[tapted, 2013-01-31 01:32:19]

On 2013/01/31 01:19:20, cpu wrote:
> You seem to know more than me about these kind of tests, I personally prefer
> simple and reliable which might be GetMaxPageID

I've gone with just GetMaxPageID. The WebUIBrowserTest option would definitely
not be simple ;)

PTAL.

[cpu_(ooo_6.6-7.5), 2013-01-31 22:49:04]

lgtm

[commit-bot: I haz the power, 2013-01-31 23:00:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/12094026/14001

[commit-bot: I haz the power, 2013-02-01 01:52:20]

Change committed as 180039