Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
447053002:
Add Origin Power Map to Store Battery Auditing Data. (Closed)
|
Created:
6 years, 4 months ago by Daniel Nishi Modified:
6 years, 4 months ago CC:
chromium-reviews, scheib, Siva Chandra, Daniel Erat Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionInitial commit for battery auditing by website origin by profile.
This is part of Website Settings resource/permissions monitoring.
Design Doc: https://docs.google.com/document/d/1oQwmj3AU4QYhTyGrYEGr6zaZhHUfx-wqUgEcQGbUU-U/edit?usp=sharing
BUG=372607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289689
Patch Set 1 #Patch Set 2 : Remove a line from GYPI that shouldn't have been in this patch. #
Total comments: 18
Patch Set 3 : #Patch Set 4 : Remove persistence. #
Total comments: 23
Patch Set 5 : Comments addressed. #
Total comments: 2
Patch Set 6 : #
Total comments: 2
Patch Set 7 : Add DCHECK_GE #Patch Set 8 : Fix a bug with uninitialized POD. #Patch Set 9 : Componentialize and remove dependency in //content. #
Total comments: 22
Patch Set 10 : Whoops, bad GN. #
Total comments: 2
Patch Set 11 : Address comments. #
Total comments: 6
Patch Set 12 : #
Total comments: 1
Patch Set 13 : Typo fix. #
Total comments: 2
Patch Set 14 : Add dependency. #Patch Set 15 : Remove from Android GN due to content deps. #Patch Set 16 : rebase. #
Messages
Total messages: 47 (0 generated)
sky@: PTAL when you get a moment.
Did this go through eng review? On Wed, Aug 6, 2014 at 2:39 PM, <dhnishi@chromium.org> wrote: > Reviewers: sky, > > Message: > sky@: PTAL when you get a moment. > > Description: > Initial commit for battery auditing by website origin by profile. > > This is part of Website Settings resource/permissions monitoring. > > Design Doc: > https://docs.google.com/document/d/1oQwmj3AU4QYhTyGrYEGr6zaZhHUfx-wqUgEcQGbUU... > > BUG=372607 > > Please review this at https://codereview.chromium.org/447053002/ > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+318, -0 lines): > A chrome/browser/power/origin_power_map.h > A chrome/browser/power/origin_power_map.cc > A chrome/browser/power/origin_power_map_factory.h > A chrome/browser/power/origin_power_map_factory.cc > A chrome/browser/power/origin_power_map_unittest.cc > M chrome/chrome_browser.gypi > M chrome/chrome_tests_unit.gypi > M chrome/common/pref_names.h > M chrome/common/pref_names.cc > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.cc:17: DictionaryPrefUpdate update(prefs_, prefs::kBatteryOriginMap); Is kBatteryOriginMap effectively going to become every url I've visited? If so, preferences isn't intended for that volume of data. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:10: #include "base/memory/ref_counted.h" Do you really need the ref_counted include? https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:31: typedef std::map<GURL, int> ScaledOriginMap; typedefs should be first in a section (see style guide). https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:31: typedef std::map<GURL, int> ScaledOriginMap; Why is this named Scaled? Percent would be more meaningful here and in the name of your getter below. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:38: void AddPowerForOrigin(const GURL& origin, double power); Document what |power| means here. It's worth a description at the class level as to how tracking is done. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:42: scoped_ptr<ScaledOriginMap> GetScaledOriginMap(); nit: const https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:56: }; DISALLOW_... https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map_unittest.cc (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map_unittest.cc:6: #include "chrome/browser/power/origin_power_map.h" nit: this should be your first include with a newline (just as you have in origin_power_map.cc).
https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.cc:17: DictionaryPrefUpdate update(prefs_, prefs::kBatteryOriginMap); On 2014/08/06 23:59:22, sky wrote: > Is kBatteryOriginMap effectively going to become every url I've visited? If so, > preferences isn't intended for that volume of data. Would replacing this with a SQLite Database be an acceptable solution? https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:10: #include "base/memory/ref_counted.h" On 2014/08/06 23:59:23, sky wrote: > Do you really need the ref_counted include? Unnecessary include removed. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:31: typedef std::map<GURL, int> ScaledOriginMap; On 2014/08/06 23:59:23, sky wrote: > typedefs should be first in a section (see style guide). Typedefs moved up. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:31: typedef std::map<GURL, int> ScaledOriginMap; On 2014/08/06 23:59:23, sky wrote: > Why is this named Scaled? Percent would be more meaningful here and in the name > of your getter below. I was thinking that since it was scaled to 100 as a percent. Percent is more accurate, though. I've changed over all instances of the word Scaled to Percent. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:38: void AddPowerForOrigin(const GURL& origin, double power); On 2014/08/06 23:59:23, sky wrote: > Document what |power| means here. It's worth a description at the class level as > to how tracking is done. Since the definition of |power| is platform specific, should the platform specific notes be included here or in the implementation by platform? https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:42: scoped_ptr<ScaledOriginMap> GetScaledOriginMap(); On 2014/08/06 23:59:23, sky wrote: > nit: const Done. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.h:56: }; On 2014/08/06 23:59:23, sky wrote: > DISALLOW_... Done. https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map_unittest.cc (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map_unittest.cc:6: #include "chrome/browser/power/origin_power_map.h" On 2014/08/06 23:59:23, sky wrote: > nit: this should be your first include with a newline (just as you have in > origin_power_map.cc). Done.
https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.cc:17: DictionaryPrefUpdate update(prefs_, prefs::kBatteryOriginMap); On 2014/08/07 21:15:08, Daniel Nishi wrote: > On 2014/08/06 23:59:22, sky wrote: > > Is kBatteryOriginMap effectively going to become every url I've visited? If > so, > > preferences isn't intended for that volume of data. > > Would replacing this with a SQLite Database be an acceptable solution? Why do you need to make this data persistent at all? Seems to me reporting data for the current session should be useful enough. That said, I also don't like that this code is potentially going to track all urls I visit and never prune. You need some sort of pruning strategy in here. Maybe tracking the active tabs with a steep fall off curve for anything that isn't active. I also think people don't necessarily care about power useage per url, rather per host.
https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/20001/chrome/browser/power/ori... chrome/browser/power/origin_power_map.cc:17: DictionaryPrefUpdate update(prefs_, prefs::kBatteryOriginMap); On 2014/08/07 21:47:07, sky wrote: > On 2014/08/07 21:15:08, Daniel Nishi wrote: > > On 2014/08/06 23:59:22, sky wrote: > > > Is kBatteryOriginMap effectively going to become every url I've visited? If > > so, > > > preferences isn't intended for that volume of data. > > > > Would replacing this with a SQLite Database be an acceptable solution? > > Why do you need to make this data persistent at all? Seems to me reporting data > for the current session should be useful enough. > That said, I also don't like that this code is potentially going to track all > urls I visit and never prune. You need some sort of pruning strategy in here. > Maybe tracking the active tabs with a steep fall off curve for anything that > isn't active. I also think people don't necessarily care about power useage per > url, rather per host. Ah, I should have specified -- it is intended to be used per-origin, not per-URL. I'll add a DCHECK to enforce that only origin URLs come in. I'll discuss the persistence of the data and come up with a more concrete invalidation plan.
sky: PTAL. Persistence have been removed for now.
Is there a reason this code needs to live in chrome? https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:20: return -1; Why the -1 here? I would have expected 0. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:21: else if (!total_consumed_) nit: no else after a return (see chromium specific style guide). https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:27: DCHECK_EQ(origin, origin.GetOrigin()); Why does the caller have to have called GetOrigin() first? Shouldn't the exact heuristics of what constitutes and origin be up to this class and not the caller? https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:28: if (!origin.is_valid() || origin.SchemeIs(content::kChromeUIScheme)) Why do you special case chrome urls? https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:31: if (origin_map_.find(origin) == origin_map_.end()) { nit: no {} https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:31: if (origin_map_.find(origin) == origin_map_.end()) { And you actually shouldn't need this if at all. origin_map_[something_you_never_added] should be 0. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:40: OriginPowerMap::PercentOriginMap* percent_map = Create a scoped_ptr here and use .Pass() in the return statement. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.h:20: class PrefRegistrySyncable; Remove all the forward declarations that are no longer needed. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.h:29: explicit OriginPowerMap(); remove explicit https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map_factory.h (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map_factory.h:25: // BrowserContextKeyedServiceFactory; ; -> : https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map_factory.h:28: }; DISALLOW_...
https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:20: return -1; On 2014/08/08 19:12:25, sky wrote: > Why the -1 here? I would have expected 0. Fair enough. If it's not in the map, it hasn't consumed any power. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:21: else if (!total_consumed_) On 2014/08/08 19:12:25, sky wrote: > nit: no else after a return (see chromium specific style guide). Changed the return value if not found to 0 and merged the if and elif. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:27: DCHECK_EQ(origin, origin.GetOrigin()); On 2014/08/08 19:12:25, sky wrote: > Why does the caller have to have called GetOrigin() first? Shouldn't the exact > heuristics of what constitutes and origin be up to this class and not the > caller? Looking over the GetOrigin() call, I agree. For one example, a GURL could be valid for the map even if these replacements[0] haven't been run. I removed the DCHECKs. [0] https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&rcl=14... https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:28: if (!origin.is_valid() || origin.SchemeIs(content::kChromeUIScheme)) On 2014/08/08 19:12:25, sky wrote: > Why do you special case chrome urls? When they appear in the resource monitoring UI, there is nothing actionable that can be done about the chrome URLs, whereas actions can be taken against individual origins or Chrome apps. It felt like the data was less polluted and gave a better idea for which sites/apps were using your resources when the chrome URLs were excluded. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:31: if (origin_map_.find(origin) == origin_map_.end()) { On 2014/08/08 19:12:25, sky wrote: > And you actually shouldn't need this if at all. > origin_map_[something_you_never_added] should be 0. Good point. I've removed this if. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:40: OriginPowerMap::PercentOriginMap* percent_map = On 2014/08/08 19:12:25, sky wrote: > Create a scoped_ptr here and use .Pass() in the return statement. I think I'd lose the const-ness if I did this, since I populate the PercentOriginMap in this function. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.h:20: class PrefRegistrySyncable; On 2014/08/08 19:12:25, sky wrote: > Remove all the forward declarations that are no longer needed. Done. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.h:29: explicit OriginPowerMap(); On 2014/08/08 19:12:25, sky wrote: > remove explicit Done. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map_factory.h (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map_factory.h:25: // BrowserContextKeyedServiceFactory; On 2014/08/08 19:12:25, sky wrote: > ; -> : Done. https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map_factory.h:28: }; On 2014/08/08 19:12:25, sky wrote: > DISALLOW_... Done.
Sorry, missed the top comment. I dropped it in Chrome since there is one map per profile, and it uses a Profile* in the factory to get the right OriginPowerMap.
https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:40: OriginPowerMap::PercentOriginMap* percent_map = On 2014/08/08 19:41:05, Daniel Nishi wrote: > On 2014/08/08 19:12:25, sky wrote: > > Create a scoped_ptr here and use .Pass() in the return statement. > > I think I'd lose the const-ness if I did this, since I populate the > PercentOriginMap in this function. There is no point in returning a const value here. The return value should be scoped_ptr<PercentOriginMap>. https://codereview.chromium.org/447053002/diff/120001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/120001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:27: origin_map_[origin] += power; I was actually saying something differently. I think you should do origin.GetOrigin() here and let people submit whatever URLs they want. Same thing in the GetPowerForOrigin() above.
https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/100001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:40: OriginPowerMap::PercentOriginMap* percent_map = On 2014/08/08 23:15:28, sky wrote: > On 2014/08/08 19:41:05, Daniel Nishi wrote: > > On 2014/08/08 19:12:25, sky wrote: > > > Create a scoped_ptr here and use .Pass() in the return statement. > > > > I think I'd lose the const-ness if I did this, since I populate the > > PercentOriginMap in this function. > > There is no point in returning a const value here. The return value should be > scoped_ptr<PercentOriginMap>. Done. https://codereview.chromium.org/447053002/diff/120001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/120001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:27: origin_map_[origin] += power; On 2014/08/08 23:15:28, sky wrote: > I was actually saying something differently. I think you should do > origin.GetOrigin() here and let people submit whatever URLs they want. Same > thing in the GetPowerForOrigin() above. Oh, I see. I misunderstood. I've allowed arbitrary URLs to be passed in and the origin is now used.
LGTM https://codereview.chromium.org/447053002/diff/140001/chrome/browser/power/or... File chrome/browser/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/140001/chrome/browser/power/or... chrome/browser/power/origin_power_map.cc:25: GURL origin = url.GetOrigin(); DCHECK_GE(power, 0)
Darin sent an email saying this doesn't look like it needs an eng review, so I'll go ahead an CQ it. Thanks for the quick reviews. https://chromiumcodereview.appspot.com/447053002/diff/140001/chrome/browser/p... File chrome/browser/power/origin_power_map.cc (right): https://chromiumcodereview.appspot.com/447053002/diff/140001/chrome/browser/p... chrome/browser/power/origin_power_map.cc:25: GURL origin = url.GetOrigin(); On 2014/08/08 23:44:56, sky wrote: > DCHECK_GE(power, 0) Done.
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/447053002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Actually, before you land I do have one more question. Why do you want to put this in src/chrome and not src/components? On Sat, Aug 9, 2014 at 2:07 AM, <commit-bot@chromium.org> wrote: > Try jobs failed on following builders: > mac_chromium_rel_swarming on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > https://codereview.chromium.org/447053002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/09 14:22:08, sky wrote: > Actually, before you land I do have one more question. Why do you want > to put this in src/chrome and not src/components? > > On Sat, Aug 9, 2014 at 2:07 AM, <mailto:commit-bot@chromium.org> wrote: > > Try jobs failed on following builders: > > mac_chromium_rel_swarming on tryserver.chromium.mac > > > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > https://codereview.chromium.org/447053002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. OriginPowerMapFactory has a dependency on Profile.
Is it Profile, or BrowserContext? On Mon, Aug 11, 2014 at 10:47 AM, <dhnishi@chromium.org> wrote: > On 2014/08/09 14:22:08, sky wrote: >> >> Actually, before you land I do have one more question. Why do you want >> to put this in src/chrome and not src/components? > > >> On Sat, Aug 9, 2014 at 2:07 AM, <mailto:commit-bot@chromium.org> wrote: >> > Try jobs failed on following builders: >> > mac_chromium_rel_swarming on tryserver.chromium.mac >> > > > > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) >> >> > >> > https://codereview.chromium.org/447053002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > OriginPowerMapFactory has a dependency on Profile. > > https://codereview.chromium.org/447053002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/11 19:19:19, sky wrote: > Is it Profile, or BrowserContext? > > On Mon, Aug 11, 2014 at 10:47 AM, <mailto:dhnishi@chromium.org> wrote: > > On 2014/08/09 14:22:08, sky wrote: > >> > >> Actually, before you land I do have one more question. Why do you want > >> to put this in src/chrome and not src/components? > > > > > >> On Sat, Aug 9, 2014 at 2:07 AM, <mailto:commit-bot@chromium.org> wrote: > >> > Try jobs failed on following builders: > >> > mac_chromium_rel_swarming on tryserver.chromium.mac > >> > > > > > > > > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > >> > >> > > >> > https://codereview.chromium.org/447053002/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > OriginPowerMapFactory has a dependency on Profile. > > > > https://codereview.chromium.org/447053002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Profile, in this case. I used this idiom to make it Profile-specific: https://code.google.com/p/chromium/codesearch#search/&q=getforprofile%20Facto...
On Mon, Aug 11, 2014 at 12:22 PM, <dhnishi@chromium.org> wrote: > On 2014/08/11 19:19:19, sky wrote: >> >> Is it Profile, or BrowserContext? I get that you're currently taking a profile, but I believe you can right directly to BrowserContext and not have any dependencies on chrome. -Scott > > >> On Mon, Aug 11, 2014 at 10:47 AM, <mailto:dhnishi@chromium.org> wrote: >> > On 2014/08/09 14:22:08, sky wrote: >> >> >> >> Actually, before you land I do have one more question. Why do you want >> >> to put this in src/chrome and not src/components? >> > >> > >> >> On Sat, Aug 9, 2014 at 2:07 AM, <mailto:commit-bot@chromium.org> >> >> wrote: >> >> > Try jobs failed on following builders: >> >> > mac_chromium_rel_swarming on tryserver.chromium.mac >> >> > >> > >> > >> > > > > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) >> >> >> >> >> > >> >> > https://codereview.chromium.org/447053002/ >> > >> > >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > >> > email >> >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > >> > >> > OriginPowerMapFactory has a dependency on Profile. >> > >> > https://codereview.chromium.org/447053002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > Profile, in this case. > > I used this idiom to make it Profile-specific: > https://code.google.com/p/chromium/codesearch#search/&q=getforprofile%20Facto... > > https://codereview.chromium.org/447053002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
blundell: PTAL at the new component. +sivachandra@ +derat@ FYI and OWNERS
https://codereview.chromium.org/447053002/diff/320001/components/power/origin... File components/power/origin_power_map_factory.h (right): https://codereview.chromium.org/447053002/diff/320001/components/power/origin... components/power/origin_power_map_factory.h:9: #include "components/keyed_service/content/browser_context_keyed_service_factory.h" Is this component intended to be used on iOS? Your avoidance of dependencies on //content suggests that it is; however, this dependency is effectively a //content dependency. You can either: (1) Move the factory back to //chrome (but keep the service in the component) (2) If this component isn't intended to be used on iOS, move it to the list of gypis not used on iOS in components.gyp, add a dependency on keyed_service_content in your gypi file, and depend on //content as much as you want :).
https://codereview.chromium.org/447053002/diff/280001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/447053002/diff/280001/components/BUILD.gn#new... components/BUILD.gn:59: "//components/power", fix sorting ("policy" < "power") https://codereview.chromium.org/447053002/diff/280001/components/power/origin... File components/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:11: const char kChromeUIScheme[] = "chrome"; duplicating a constant to avoid a dependency feels gross. do any other things outside of content/ need to deal with this? https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:15: OriginPowerMap::OriginPowerMap() : total_consumed_() { explicitly initialize |total_consumed_| to 0.0 https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:25: return origin_map_[origin] * 100 / total_consumed_; use an iterator so you don't do the lookup twice. might as well also skip the lookup if total_consumed_ is 0. and you should probably round the result, right? if (!total_consumed_) return 0; OriginMap::const_iterator it = origin_map_.find(origin); return it == origin_map_.end() ? 0 : static_cast<int>(it->second * 100 / total_consumed_ + 0.5); https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:43: ++it) { don't you need to return here if total_consumed_ is 0 to avoid a divide-by-zero, as in GetPowerForOrigin()? please add a test for this. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:44: (*percent_map)[it->first] = (it->second / total_consumed_ * 100); round here too https://codereview.chromium.org/447053002/diff/280001/components/power/origin... File components/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:28: // Add a certain amount of power consumption to a given URL's origin. nit: s/Add/Adds/ https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:32: // Return a map of all origins to the integer percentage usage of power nit: s/Return/Returns/ https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:34: scoped_ptr<PercentOriginMap> GetPercentOriginMap(); does this need to be scoped_ptr? i'm wondering if we can just depend on http://en.wikipedia.org/wiki/Return_value_optimization here. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:37: typedef std::map<GURL, double> OriginMap; nit: add a comment documenting what the doubles are here ([0.0, 100.0]? [0.0, 1.0]? fractions of |total_consumed_|, or actual power consumed? https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:40: // The total amount of power consumed since the last wipe. when do "wipes" happen? i don't see |total_consumed_| getting reset in the implementation. does the entire OriginPowerMap get thrown away periodically?
https://codereview.chromium.org/447053002/diff/280001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/447053002/diff/280001/components/BUILD.gn#new... components/BUILD.gn:59: "//components/power", On 2014/08/12 15:58:51, Daniel Erat wrote: > fix sorting ("policy" < "power") Done. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... File components/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:11: const char kChromeUIScheme[] = "chrome"; On 2014/08/12 15:58:52, Daniel Erat wrote: > duplicating a constant to avoid a dependency feels gross. do any other things > outside of content/ need to deal with this? There is precedent to avoid dependencies on //content to duplicate constants, however since we're not on iOS and have a content dependency already I've changed this back to using the regular constant. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:15: OriginPowerMap::OriginPowerMap() : total_consumed_() { On 2014/08/12 15:58:52, Daniel Erat wrote: > explicitly initialize |total_consumed_| to 0.0 Done. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:25: return origin_map_[origin] * 100 / total_consumed_; On 2014/08/12 15:58:52, Daniel Erat wrote: > use an iterator so you don't do the lookup twice. > > might as well also skip the lookup if total_consumed_ is 0. > > and you should probably round the result, right? > > if (!total_consumed_) > return 0; > > OriginMap::const_iterator it = origin_map_.find(origin); > return it == origin_map_.end() ? 0 : > static_cast<int>(it->second * 100 / total_consumed_ + 0.5); Done. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:43: ++it) { On 2014/08/12 15:58:52, Daniel Erat wrote: > don't you need to return here if total_consumed_ is 0 to avoid a divide-by-zero, > as in GetPowerForOrigin()? please add a test for this. This is true. If all sources in the map consumed no power, it's effectively like saying nothing did, so I've added a return of an empty map here. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.cc:44: (*percent_map)[it->first] = (it->second / total_consumed_ * 100); On 2014/08/12 15:58:52, Daniel Erat wrote: > round here too Done. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... File components/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:28: // Add a certain amount of power consumption to a given URL's origin. On 2014/08/12 15:58:52, Daniel Erat wrote: > nit: s/Add/Adds/ Done. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:32: // Return a map of all origins to the integer percentage usage of power On 2014/08/12 15:58:52, Daniel Erat wrote: > nit: s/Return/Returns/ Done. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:34: scoped_ptr<PercentOriginMap> GetPercentOriginMap(); On 2014/08/12 15:58:52, Daniel Erat wrote: > does this need to be scoped_ptr? i'm wondering if we can just depend on > http://en.wikipedia.org/wiki/Return_value_optimization here. I didn't do any formal profiling, but my unittests don't run any slower with just returning the value. I'll just return the value and rely on that. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:37: typedef std::map<GURL, double> OriginMap; On 2014/08/12 15:58:52, Daniel Erat wrote: > nit: add a comment documenting what the doubles are here ([0.0, 100.0]? [0.0, > 1.0]? fractions of |total_consumed_|, or actual power consumed? Done. https://codereview.chromium.org/447053002/diff/280001/components/power/origin... components/power/origin_power_map.h:40: // The total amount of power consumed since the last wipe. On 2014/08/12 15:58:52, Daniel Erat wrote: > when do "wipes" happen? i don't see |total_consumed_| getting reset in the > implementation. does the entire OriginPowerMap get thrown away periodically? Since the data is not currently persisted, this will occur when the browser shuts down. That said, we want battery persistence of some sort in the future and the exact invalidation spec is being discussed on the design doc. https://codereview.chromium.org/447053002/diff/320001/components/power/origin... File components/power/origin_power_map_factory.h (right): https://codereview.chromium.org/447053002/diff/320001/components/power/origin... components/power/origin_power_map_factory.h:9: #include "components/keyed_service/content/browser_context_keyed_service_factory.h" On 2014/08/12 12:49:00, blundell wrote: > Is this component intended to be used on iOS? Your avoidance of dependencies on > //content suggests that it is; however, this dependency is effectively a > //content dependency. You can either: > (1) Move the factory back to //chrome (but keep the service in the component) > (2) If this component isn't intended to be used on iOS, move it to the list of > gypis not used on iOS in components.gyp, add a dependency on > keyed_service_content in your gypi file, and depend on //content as much as you > want :). Sadly, iOS is the only platform that doesn't implement what I need to have a power heuristic. I've gone with the second option.
https://codereview.chromium.org/447053002/diff/360001/components/power/DEPS File components/power/DEPS (right): https://codereview.chromium.org/447053002/diff/360001/components/power/DEPS#n... components/power/DEPS:2: "+content/public/common", fix sorting here too https://codereview.chromium.org/447053002/diff/360001/components/power/origin... File components/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/360001/components/power/origin... components/power/origin_power_map.cc:25: ? 0 this should just be indented four spaces beyond the previous line. operators are usually at the end of the previous line, too (see my earlier suggestion) https://codereview.chromium.org/447053002/diff/360001/components/power/origin... File components/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/360001/components/power/origin... components/power/origin_power_map.h:37: // OriginMap maps a URL to the platform specific metric of power consumed. could you be more specific about how these values relate to total_consumed_ in particular? for example: // OriginMap maps a URL to the amount of power consumed by // that URL using the same units as |total_consumed_|. typedef ... // Total amount of power consumed using opaque platform-specific // units. double total_consumed_;
https://codereview.chromium.org/447053002/diff/360001/components/power/DEPS File components/power/DEPS (right): https://codereview.chromium.org/447053002/diff/360001/components/power/DEPS#n... components/power/DEPS:2: "+content/public/common", On 2014/08/12 18:40:18, Daniel Erat wrote: > fix sorting here too Done. https://codereview.chromium.org/447053002/diff/360001/components/power/origin... File components/power/origin_power_map.cc (right): https://codereview.chromium.org/447053002/diff/360001/components/power/origin... components/power/origin_power_map.cc:25: ? 0 On 2014/08/12 18:40:18, Daniel Erat wrote: > this should just be indented four spaces beyond the previous line. operators are > usually at the end of the previous line, too (see my earlier suggestion) Whoops. An errant git cl format knocked out the formatting. Fixed. https://codereview.chromium.org/447053002/diff/360001/components/power/origin... File components/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/360001/components/power/origin... components/power/origin_power_map.h:37: // OriginMap maps a URL to the platform specific metric of power consumed. On 2014/08/12 18:40:18, Daniel Erat wrote: > could you be more specific about how these values relate to total_consumed_ in > particular? for example: > > // OriginMap maps a URL to the amount of power consumed by > // that URL using the same units as |total_consumed_|. > typedef ... > > // Total amount of power consumed using opaque platform-specific > // units. > double total_consumed_; I've upped the comments.
lgtm after fixing a typo https://codereview.chromium.org/447053002/diff/380001/components/power/origin... File components/power/origin_power_map.h (right): https://codereview.chromium.org/447053002/diff/380001/components/power/origin... components/power/origin_power_map.h:37: // OriginMap maps a URL to the pamount of power consumed by the URL using the nit: s/pamount/amount/
LGTM - thanks for moving!
lgtm https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS File components/power/DEPS (right): https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS#n... components/power/DEPS:3: "+content/public/common", This should be a dependency as well.
avi: PTAL as OWNER of content/public/common added to DEPS? https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS File components/power/DEPS (right): https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS#n... components/power/DEPS:3: "+content/public/common", On 2014/08/13 07:00:24, blundell wrote: > This should be a dependency as well. Added as dep.
On 2014/08/13 17:03:35, Daniel Nishi wrote: > avi: PTAL as OWNER of content/public/common added to DEPS? > > https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS > File components/power/DEPS (right): > > https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS#n... > components/power/DEPS:3: "+content/public/common", > On 2014/08/13 07:00:24, blundell wrote: > > This should be a dependency as well. > > Added as dep. I guess that's OK. I don't know about the dependencies of components/. It's allowed to depend on content, yes?
On 2014/08/13 17:38:21, Avi wrote: > On 2014/08/13 17:03:35, Daniel Nishi wrote: > > avi: PTAL as OWNER of content/public/common added to DEPS? > > > > https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS > > File components/power/DEPS (right): > > > > > https://codereview.chromium.org/447053002/diff/400001/components/power/DEPS#n... > > components/power/DEPS:3: "+content/public/common", > > On 2014/08/13 07:00:24, blundell wrote: > > > This should be a dependency as well. > > > > Added as dep. > > I guess that's OK. I don't know about the dependencies of components/. It's > allowed to depend on content, yes? As long as the component isn't used in iOS, it's allowed. As of right now, this won't get built into iOS.
Well, LGTM then for now.
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/447053002/420001
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/447053002/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46671) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46783) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/447053002/480001
Message was sent while issue was closed.
Committed patchset #16 (480001) as 289689 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
