UI: Create BraveActionsContainer and add Shields extension button to it #343

petemill · 2018-08-20T03:46:13Z

Intention is for Shields 'extension' to feel built-in.
BraveActionsContainer hooks in to extension system in order to display and update the button icon when necessary.
Removes the Shields extension button from main extensions area (ToolbarActionsBar / BrowserActionsContainer).
Disable context menu for shields extension button in brave actions area since there are no valid actions.
Patches the content of a method at LocationBarView::Layout since it could be more harmful to copy that functionality to our subclass because it is complex and somewhat functional logic. Attempts to minimize the patching necessary by passing in a new argument to the method.

This PR goes hand-in-hand with brave/brave-extension#57 which controls the badge color, restricts the text from growing too wide ("99+") and uses a vector icon for better rendering. <-- Now merged

BraveActionsContainer will also contain the Brave Rewards button, which will be implemented in a subsequent PR, as either a button observing the Rewards Service, or another extension with a browser action.

A subsequent PR will implement a less-rounded Location Bar that will prevent cutting off the badge:

With this PR only it looks like this:

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

petemill · 2018-08-28T05:29:25Z

This is now ready for review

simonhong · 2018-08-28T23:02:47Z

Sorry for late. I'll review today.

simonhong · 2018-08-29T03:43:10Z

browser/ui/toolbar/brave_toolbar_actions_model.cc

+
+#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
+#include "extensions/common/extension.h"
+#include "brave/common/extensions/extension_constants.h"


simonhong · 2018-08-29T03:48:10Z

browser/ui/brave_actions/brave_action_icon_with_badge_image_source.cc

+    int w = 0;
+    int h = 0;
+    gfx::FontList bigger_font =
+        base_font.Derive(1, 0, gfx::Font::Weight::NORMAL);


How about using base_font.DeriveWithSizeDelta(1) because we just want resizing only?
Current one might change style and weight.

This was copied from chromium implementation. I agree that maybe it's better, though perhaps it's best to stay the same as chromium in order to compare against future versions.

simonhong · 2018-08-29T03:51:28Z

browser/ui/brave_actions/brave_action_icon_with_badge_image_source.h

+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/canvas.h"
+
+


nit: two empty line.

simonhong · 2018-08-29T04:07:28Z

browser/ui/brave_actions/brave_action_icon_with_badge_image_source.cc

+
+  constexpr int kBadgeHeight = 12;
+  constexpr int kPadding = 2;
+  const int kTextHeight = 2 + kBadgeHeight - (kPadding * 2);


What is first 2 means?

The numbers were generally rendering smaller than the height so I added some ability to bleed in to padding area. But it would be cleaner to just reduce padding on height to 1, so thanks.

simonhong · 2018-08-29T04:44:13Z

browser/ui/brave_actions/brave_action_icon_with_badge_image_source.cc

+    gfx::FontList bigger_font =
+        base_font.Derive(1, 0, gfx::Font::Weight::NORMAL);
+    gfx::Canvas::SizeStringInt(utf16_text, bigger_font, &w, &h, 0,
+                               gfx::Canvas::NO_ELLIPSIS);


Just curiosity, did NO_ELLIPSIS affect height calculation?

Didn't try - this was copied from chromium icon_with_badge_image_source

simonhong · 2018-08-29T06:07:08Z

browser/ui/views/brave_actions/brave_actions_container.h

+
+    bool should_hide_ = false;
+
+    ToolbarActionView* shields_button_view_ = nullptr;


How about using std::unique_ptr for |shields_button_view_|? because this class owns it.
In chromium, using raw point means it doesn't own it.

I copied this pattern from chromium, for example at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/location_bar_view.h?type=cs&q=LocationBarView&sq=package:chromium&g=0&l=456

Could you let me know what the difference is?

It is different. Child view you referenced is owned by parent view. That means child view is destroyed when parent view destroyed.
In our case, we delete it explicitly. That means deleting can happen from view tree or our delete.
In our case, it could be possible double delete.

Re-reading what chromium does for dynamic extension browser action icons: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/browser_actions_container.cc?type=cs&sq=package:chromium&g=0&l=158-179

Will consider not deleteing the view manually and maybe just RemoveChildView, or using shields_button_view_->set_owned_by_client() as suggested below.

Yep, I think RemoveChildView is more safer than delete.

I read again and I think worried situation(double delete) would not be happened.
Explicit deleting is done when OnExtensionUnloaded() is called and implicit deleting is done when parent(this class) is destroyed.
When parent is destroyed first, OnExtensionUnloaded() cannot be called again.
When OnExtnsionUnloaded() is called first, button view is removed from parent and it is explicitly deleted.
So, I think current impl doesn't have problem. WDYT?

Yes @simonhong after thinking again I think we should keep the delete here since after calling RemoveChildView the view will never get deleted by the old parent, so we will never free the memory. And we cannot re-use the ToolbarActionView for another instance of the extension.

simonhong · 2018-08-29T06:08:31Z

browser/ui/views/brave_actions/brave_actions_container.h

+    extensions::ExtensionActionAPI* extension_action_api_;
+    extensions::ExtensionRegistry* extension_registry_;
+    extensions::ExtensionActionManager* extension_action_manager_;
+    ToolbarActionViewController* shields_view_controller_ = nullptr;


same as shields_button_view_.

I think we should consider using smart pointer to |shields_view_controller_|.
Or if we can guarantee RemoveShields() is always called before BraveActionsContainer is destroyed,
just adding DCHECK(!shields_view_controller_) in dtor would be sufficient.

simonhong · 2018-08-29T06:10:43Z

browser/ui/views/brave_actions/brave_actions_container.cc

+        browser_, extension_action_manager_->GetExtensionAction(*brave_extension),
+                                                                        nullptr);
+    shields_button_view_ = new ToolbarActionView(
+                                                  shields_view_controller_, this);


Let's call shields_button_view_->set_owned_by_client() because parent doesn't own button view.
With this, parent can't delete it by accident.

I'm confused about this - BraveActionsContainer is the parent, since we add it as a child, right? If the Location Bar is deleted, it's ok to delete this button, no?

Ah,,, I'm concerning there are two different delete path.(by tree view or by our delete).
That can cause some problem like double delete.

oh i see, thanks. If I call that, does that mean I need to delete in destructor too?

(FYI I was trying to avoid double delete by setting to nullptr but perhaps it can be deleted by tree view first?)

Right. I worried that situation.
So, smart pointer can handle it.

simonhong · 2018-08-29T06:30:29Z

browser/ui/views/brave_actions/brave_actions_container.cc

+  if (shields_view_controller_)
+    shields_view_controller_->UpdateState();
+  // only show separator if we're showing any buttons
+  const bool visible = !should_hide_ && shields_button_view_;


How about checking children count() instead of checking |shields_button_view_| pointer?
In the near future, I assume that this view can have more buttons.
Because of this, I think we should not assume this view can have only one button.

Interesting idea. My intention was to add the one other button (Brave Rewards) to this conditional, once the button is added. I think checking explicitly what we require to be visible is appropriate since we may have a changing number of visual-only elements, such as the separator.

simonhong · 2018-08-29T06:31:52Z

browser/ui/views/brave_actions/brave_actions_container.cc

+  if (shields_button_view_) {
+    RemoveChildView(shields_button_view_);
+    delete shields_button_view_;
+    delete shields_view_controller_;


same as below comment (children count)

simonhong · 2018-08-29T06:53:25Z

browser/ui/views/location_bar/brave_location_bar_view.cc

+
+#include "chrome/browser/ui/layout_constants.h"
+#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "brave/browser/ui/views/brave_actions/brave_actions_container.h"


simonhong · 2018-08-29T06:53:39Z

browser/ui/views/location_bar/brave_location_bar_view.cc

+#include "chrome/browser/ui/location_bar/location_bar.h"
+#include "brave/browser/ui/location_bar/brave_location_bar.h"
+
+#define LocationBar BraveLocationBar


what is this for?

We were previously overriding LocationBar, a subclass of LocationBarView via a chromium_src override in LocationBarView, but now we can do it in this class instead.

simonhong · 2018-08-29T07:06:25Z

browser/ui/views/brave_actions/brave_actions_container.h

+#ifndef BRAVE_BROWSER_UI_VIEWS_BRAVE_ACTIONS_BRAVE_ACTIONS_CONTAINER_H_
+#define BRAVE_BROWSER_UI_VIEWS_BRAVE_ACTIONS_BRAVE_ACTIONS_CONTAINER_H_
+
+


nit: two empty line.

simonhong · 2018-08-29T07:23:55Z

patches/chrome-browser-ui-views-location_bar-location_bar_view.cc.patch

+
+  if (right_most && right_most->visible())
+    trailing_decorations.AddDecoration(0,
+                              location_height + (vertical_padding * 2),


how about using height() instead of location_height + (vertical_padding * 2)?
I think both are same.

simonhong · 2018-08-29T07:30:41Z

browser/ui/views/location_bar/brave_location_bar_view.cc

+gfx::Size BraveLocationBarView::CalculatePreferredSize() const {
+  gfx::Size min_size = LocationBarView::CalculatePreferredSize();
+  if (brave_actions_ && brave_actions_->visible()){
+    int brave_actions_min = brave_actions_->GetMinimumSize().width();


I think GetLayoutConstant(LOCATION_BAR_ELEMENT_PADDING) value should be also considered.

petemill · 2018-08-30T01:32:54Z

ok I have addressed all the review comments - thanks @simonhong

I've also introduced the following concept after talking with the design team:

Previously, the badge text font-size grew to meet a target height
Now we introduce a max-width to the badge, and first decrease the font-size until that width is met.
If the text was always more narrow than the max-width, then we attempt to increase the font-size to match the target height, as we did before.

This ensures that the logo is not covered as much.

Fix brave/brave-browser#668 Intention is for Shields 'extension' to feel built-in. BraveActionsContainer hooks in to extension system in order to display and update the button icon when necessary. Displays a custom badge which is sized and positioned specifically for the Shields count. Uses dynamic font sizing to maintain a target height and maximum width. Removes the Shields extension button from main extensions area (ToolbarActionsBar / BrowserActionsContainer). Disable context menu for shields extension button in brave actions area since there are no valid actions. Patches the content of a method at LocationBarView::Layout since it could be more harmful to copy that functionality to our subclass because it is complex and somewhat functional logic. Attempts to minimize the patching necessary by passing in a new argument to the method.

simonhong

LGTM 👍

bbondy · 2018-08-31T13:40:42Z

chromium_src/chrome/browser/ui/toolbar/toolbar_actions_model_factory.cc

+#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
+#include "brave/browser/ui/toolbar/brave_toolbar_actions_model.h"
+
+#define ToolbarActionsModel BraveToolbarActionsModel


Always #undef after using #define and doing the include.

This can't be used because the base class has a static function with that return type in the file you're directly patching.

bbondy · 2018-08-31T13:40:52Z

chromium_src/chrome/browser/ui/views/toolbar/toolbar_view.cc

+#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "brave/browser/ui/views/location_bar/brave_location_bar_view.h"
+
+#define LocationBarView BraveLocationBarView


petemill added the UI label Aug 20, 2018

petemill self-assigned this Aug 20, 2018

petemill added the pr/work-in-progress label Aug 20, 2018

bbondy changed the title ~~UI: Create BraveActionsContainer and add Shields extension button to it~~ WIP: UI: Create BraveActionsContainer and add Shields extension button to it Aug 21, 2018

petemill force-pushed the ui/shields-button-location-bar branch 6 times, most recently from 7d6b028 to a3679a8 Compare August 28, 2018 05:26

petemill requested a review from simonhong August 28, 2018 05:28

petemill removed the pr/work-in-progress label Aug 28, 2018

petemill requested a review from bbondy August 28, 2018 05:28

petemill changed the title ~~WIP: UI: Create BraveActionsContainer and add Shields extension button to it~~ UI: Create BraveActionsContainer and add Shields extension button to it Aug 28, 2018

petemill force-pushed the ui/shields-button-location-bar branch 2 times, most recently from 6383b72 to b9c2f5c Compare August 28, 2018 05:43

petemill force-pushed the ui/shields-button-location-bar branch from b9c2f5c to 7453556 Compare August 28, 2018 23:18

simonhong reviewed Aug 29, 2018

View reviewed changes

petemill force-pushed the ui/shields-button-location-bar branch from 7453556 to 00935bd Compare August 30, 2018 01:30

petemill force-pushed the ui/shields-button-location-bar branch from 00935bd to f352ef9 Compare August 30, 2018 02:12

simonhong approved these changes Aug 30, 2018

View reviewed changes

petemill merged commit 691ff55 into brave:master Aug 30, 2018

yrliou mentioned this pull request Aug 31, 2018

Link error in Windows builds brave/brave-browser#895

Closed

bbondy reviewed Aug 31, 2018

View reviewed changes

bsclifton mentioned this pull request Jul 4, 2019

The UI should allow for advanced customization brave/brave-browser#774

Open

bsclifton mentioned this pull request Jan 13, 2020

Shields should be visible when Brave is used in PWA context brave/brave-browser#7715

Open


		bool should_hide_ = false;

		ToolbarActionView* shields_button_view_ = nullptr;

		#ifndef BRAVE_BROWSER_UI_VIEWS_BRAVE_ACTIONS_BRAVE_ACTIONS_CONTAINER_H_
		#define BRAVE_BROWSER_UI_VIEWS_BRAVE_ACTIONS_BRAVE_ACTIONS_CONTAINER_H_

		#include "ui/gfx/geometry/rect.h"
		#include "ui/gfx/canvas.h"

UI: Create BraveActionsContainer and add Shields extension button to it #343

UI: Create BraveActionsContainer and add Shields extension button to it #343

Conversation

petemill commented Aug 20, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

petemill commented Aug 28, 2018

simonhong commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Aug 30, 2018

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Aug 20, 2018 •

edited

Loading

simonhong Aug 29, 2018 •

edited

Loading

simonhong Aug 29, 2018 •

edited

Loading