[Fallback icons] Change "explicit flow" interface so color hex strings don't use "#".

[Fallback icons] Change "explicit flow" interface so color hex strings don't use "#".

Design: go/chrome-fallback-icons

Fallback icon "explicit flow" URL used to have '#' in hex colors, e.g.:

chrome://fallback-icon/,#01f,#123456,,/http://www.google.com

Problem: "#" denotes fragment in URLs, so we get rid of it. This causes no ambiguity
with named color (e.g., "red") because no named color consists of letters a-f only.
Also adding "ARGB" hex color since this is supported in Skia color parsing.

BUG=455063
Committed: https://crrev.com/8fa91aabefbe9784eca0c169bc2ad2dd92f5d887
Cr-Commit-Position: refs/heads/master@{#319630}

[huangs, 2015-03-06 22:09:56]

huangs@chromium.org changed reviewers:
+ jhawkins@chromium.org

[huangs, 2015-03-06 22:09:57]

OWNER review to jhawkins, PTAL.

[James Hawkins, 2015-03-06 22:24:52]

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:16: bool
IsHexColorString(const std::string& color_str) {
Please add documentation about the conditions which make a string a hex color.

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:94: // Exclude empty case and
disallow '#' prefix since it messes up URL.
Please clarify 'messes up URL'.

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser_unittest.cc (right):

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser_unittest.cc:78: "ABCDEFH",
Can you add a negative test just for #, e.g., #ABCDEF?

[huangs, 2015-03-06 22:39:49]

Updated, PTAL.

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:16: bool
IsHexColorString(const std::string& color_str) {
On 2015/03/06 22:24:52, James Hawkins wrote:
> Please add documentation about the conditions which make a string a hex color.

Done.

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:94: // Exclude empty case and
disallow '#' prefix since it messes up URL.
On 2015/03/06 22:24:52, James Hawkins wrote:
> Please clarify 'messes up URL'.

Done. Also note that escaping '#' as "%23" would be yucky in our usage.

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser_unittest.cc (right):

https://codereview.chromium.org/988863002/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser_unittest.cc:78: "ABCDEFH",
On 2015/03/06 22:24:52, James Hawkins wrote:
> Can you add a negative test just for #, e.g., #ABCDEF?

Done.

[James Hawkins, 2015-03-06 23:47:25]

lgtm

[huangs, 2015-03-07 04:22:45]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-07 04:22:59]

Thanks. Committing!

[commit-bot: I haz the power, 2015-03-07 04:23:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988863002/40001

[commit-bot: I haz the power, 2015-03-07 06:24:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 06:24:17]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[huangs, 2015-03-08 20:43:20]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-03-08 20:44:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988863002/40001

[commit-bot: I haz the power, 2015-03-08 21:37:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-08 21:37:07]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-08 21:37:08]

The CQ bit was unchecked by commit-bot@chromium.org

[huangs, 2015-03-09 04:23:27]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-09 04:23:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
jhawkins@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/988863002/#ps60001
(title: "Sync.")

[commit-bot: I haz the power, 2015-03-09 04:24:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988863002/60001

[commit-bot: I haz the power, 2015-03-09 06:03:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-09 06:03:41]

Failed to apply the patch.

[huangs, 2015-03-09 14:58:11]

This is very weird, I'm able to do "git cl patch 988863002" on ToT.  Retrying.

[huangs, 2015-03-09 14:58:15]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-03-09 14:58:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988863002/60001

[commit-bot: I haz the power, 2015-03-09 14:59:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-09 15:00:15]

Patchset 4 (id:??) landed as
https://crrev.com/8fa91aabefbe9784eca0c169bc2ad2dd92f5d887
Cr-Commit-Position: refs/heads/master@{#319630}

[Bence, 2015-03-09 17:45:36]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/989133003/ by bnc@chromium.org.

The reason for reverting is: This seems to be causing FallbackIconUrlParserTest
unit_tests to fail, see, for example,
https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests...
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....

[Nico, 2015-03-09 18:40:43]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-03-09 18:40:43]

This breaks the valgrind bots:

Command:
/mnt/data/b/build/slave/chromium-rel-linux-valgrind-tests-3/build/src/out/Release/unit_tests
--gtest_print_time --single-process-tests
--gtest_filter=-ClientSideDetectionHostTest.FAILS_OnPhishingDetectionDoneDisabled:VisitedLinkRelayTest.Basics:NativeMessagingTest.*:ClientSideDetectionHostTest.FLAKY_OnPhishingDetectionDoneInvalidVerdict:ExtensionPrefsDelayedInstallInfo.FLAKY_DelayedInstallInfo:ProfileSyncServiceSessionTest.WriteFilledSessionToNode:ClientSideDetectionHostTest.FAILS_NavigationCancelsShouldClassifyUrl:PredictorTest.FAILS_MassiveConcurrentLookupTest:ClientSideDetectionHostTest.FLAKY_OnPhishingDetectionDoneNotPhishing:P2PTransportImplTest.FLAKY_ConnectTcp:P2PTransportImplTest.SendDataTcp:ExtensionIconManagerTest.FLAKY_LoadComponentExtensionResource:ExtensionPathUtilTest.FLAKY_BasicPrettifyPathTest:BackgroundApplicationListModelTest.FAILS_RandomTest:P2PTransportImplTest.FAILS_ConnectTcp:CloudPrintURLFetcherOverloadTest.Protect:ProcessWatcherTest.FAILS_ImmediateTermination:CloudPrintURLFetcherOverloadTest.FLAKY_Protect:AutocompleteActionPredictorTest.FAILS_RecommendActionURL:ClientSideDetectionHostTest.FAILS_OnPhishingDetectionDoneNotPhishing:CloudPrintURLFetcherBasicTest.FLAKY_HandleRawData:ExtensionIconManagerTest.FAILS_LoadComponentExtensionResource:ConnectionTesterTest.FAILS_RunAllTests:SignedSettingsTest.FLAKY_StorePolicyNoPolicyData:BackgroundApplicationListModelTest.RandomTest:VisitedLinkEventsTest.FLAKY_Coalescense:SyncFileSystemServiceTest.FAILS_SimpleLocalSyncFlow:ClientSideDetectionHostTest.OnPhishingDetectionDoneDisabled:P2PTransportImplTest.FAILS_ConnectUdp:NTPUserDataLoggerTest.FAILS_TestLogging:ExtensionUpdaterTest.TestMultipleManifestDownloading:AutocompleteActionPredictorTest.FLAKY_RecommendActionURL:AutocompleteActionPredictorTest.RecommendActionURL:VisitedLinkEventsTest.Coalescense:ExtensionUpdaterTest.FLAKY_TestMultipleManifestDownloading:VisitedLinkRelayTest.FAILS_Basics:ExtensionPrefsDelayedInstallInfo.DelayedInstallInfo:ProfileSyncServiceSessionTest.ValidTabs:CloudPrintURLFetcherBasicTest.FAILS_HandleRawData:ExtensionServiceTest.*:ClientSideDetectionHostTest.FAILS_OnPhishingDetectionDoneInvalidVerdict:PredictorTest.MassiveConcurrentLookupTest:VisitedLinkRelayTest.FLAKY_Basics:ProxyConfigServiceImplTest.*:SyncFileSystemServiceTest.SimpleLocalSyncFlow:ClientSideDetectionHostTest.FLAKY_OnPhishingDetectionDoneVerdictNotPhishing:P2PTransportImplTest.Create:ExtensionUpdaterTest.FAILS_TestMultipleManifestDownloading:RenderViewTest.FLAKY_OnHandleKeyboardEvent:URLFetcherBadHTTPSTest.FLAKY_BadHTTPSTest:P2PTransportImplTest.FLAKY_SendDataUdp:ConnectionTesterTest.FLAKY_RunAllTests:ClientSideDetectionHostTest.OnPhishingDetectionDoneInvalidVerdict:ProcessWatcherTest.FLAKY_ImmediateTermination:ProfileSyncServiceSessionTest.FLAKY_WriteFilledSessionToNode:ProfileSyncServiceSessionTest.FLAKY_ValidTabs:ExtensionIconManagerTest.LoadComponentExtensionResource:RenderViewTest.FAILS_OnHandleKeyboardEvent:P2PTransportImplTest.FLAKY_SendDataTcp:ProcessWatcherTest.ImmediateTermination:P2PTransportImplTest.ConnectUdp:RenderViewTest.FLAKY_ImeComposition:P2PTransportImplTest.FLAKY_ConnectUdp:P2PTransportImplTest.SendDataUdp:VisitedLinkEventsTest.FAILS_Coalescense:ClientSideDetectionHostTest.NavigationCancelsShouldClassifyUrl:ClientSideDetectionHostTest.OnPhishingDetectionDoneVerdictNotPhishing:CloudPrintURLFetcherBasicTest.HandleRawData:BackgroundApplicationListModelTest.FLAKY_RandomTest:ConnectionTesterTest.RunAllTests:ConnectionTesterTest.FAILS_DeleteWhileInProgress:ExtensionPrefsDelayedInstallInfo.FAILS_DelayedInstallInfo:SignedSettingsTest.StorePolicyNoPolicyData:ProfileSyncServiceSessionTest.FAILS_WriteFilledSessionToNode:RenderViewTest.OnHandleKeyboardEvent:P2PTransportImplTest.FAILS_SendDataTcp:ClientSideDetectionHostTest.FLAKY_OnPhishingDetectionDoneDisabled:URLFetcherBadHTTPSTest.FAILS_BadHTTPSTest:PredictorTest.FLAKY_MassiveConcurrentLookupTest:ExtensionWebRequestTest.*:RenderViewTest.ImeComposition:NTPUserDataLoggerTest.FLAKY_TestLogging:SyncFileSystemServiceTest.FLAKY_SimpleLocalSyncFlow:ConnectionTesterTest.DeleteWhileInProgress:P2PTransportImplTest.FAILS_Create:ExtensionPathUtilTest.FAILS_BasicPrettifyPathTest:RenderViewTest.FAILS_ImeComposition:ClientSideDetectionHostTest.FLAKY_NavigationCancelsShouldClassifyUrl:ExtensionAlarmsTest.*:P2PTransportImplTest.ConnectTcp:CloudPrintURLFetcherOverloadTest.FAILS_Protect:URLFetcherBadHTTPSTest.BadHTTPSTest:P2PTransportImplTest.FLAKY_Create:ClientSideDetectionHostTest.FAILS_OnPhishingDetectionDoneVerdictNotPhishing:P2PTransportImplTest.FAILS_SendDataUdp:StorageInfoProviderTest.*:ClientSideDetectionHostTest.OnPhishingDetectionDoneNotPhishing:SignedSettingsTest.FAILS_StorePolicyNoPolicyData:CpuInfoProviderTest.*:ProfileSyncServiceSessionTest.FAILS_ValidTabs:NTPUserDataLoggerTest.TestLogging:ExtensionPathUtilTest.BasicPrettifyPathTest:ConnectionTesterTest.FLAKY_DeleteWhileInProgress
--test-tiny-timeout=1000
InvalidRead
Invalid read of size 1
chrome::ParsedFallbackIconPath::ParseColor(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&, unsigned int*)
(chrome/common/favicon/fallback_icon_url_parser.cc:108)
chrome::ParsedFallbackIconPath::ParseSpecs(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&, int*,
favicon_base::FallbackIconStyle*)
(chrome/common/favicon/fallback_icon_url_parser.cc:80)
chrome::FallbackIconUrlParserTest::ParseSpecs(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&, int*,
favicon_base::FallbackIconStyle*)
(/mnt/data/b/build/slave/chromium-rel-linux-valgrind-tests-3/build/src/out/Release/unit_tests)
chrome::FallbackIconUrlParserTest_ParseSpecsPartial_Test::TestBody()
(chrome/common/favicon/fallback_icon_url_parser_unittest.cc:102)
Address 0x1e8adf2c is 28 bytes inside a block of size 29 free'd
operator delete(void*) (m_replacemalloc/vg_replace_malloc.c:1149)
std::basic_string<char, std::char_traits<char>, std::allocator<char>
>::~basic_string()
(/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.h:235)
chrome::ParsedFallbackIconPath::ParseColor(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&, unsigned int*)
(chrome/common/favicon/fallback_icon_url_parser.cc:104)
chrome::ParsedFallbackIconPath::ParseSpecs(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&, int*,
favicon_base::FallbackIconStyle*)
(chrome/common/favicon/fallback_icon_url_parser.cc:80)
chrome::FallbackIconUrlParserTest::ParseSpecs(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&, int*,
favicon_base::FallbackIconStyle*)
(/mnt/data/b/build/slave/chromium-rel-linux-valgrind-tests-3/build/src/out/Release/unit_tests)
chrome::FallbackIconUrlParserTest_ParseSpecsPartial_Test::TestBody()
(chrome/common/favicon/fallback_icon_url_parser_unittest.cc:102)

Is that very easy to fix, or should we revert for now?

[Nico, 2015-03-09 19:42:30]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/989183003/ by thakis@chromium.org.

The reason for reverting is: Caused invalid reads on valgrind bots, see
https://codereview.chromium.org/988863002/#msg28.

[huangs, 2015-03-09 19:44:11]

I thought this has already been reverted?