All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=841340

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=954f6d4bcd2c6aa4fe6d62b7ddaa7e5be224838f2ade04bbb5afad7fe7975b9e


Bot(s) for this bug's original alert(s):

chromium-rel-mac11-air
chromium-rel-mac12

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/1598e0cfc40000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/1598e0cfc40000

[Fullscreen Control] Integrate FullscreenControlHost with Mac Cocoa toolkit by yuweih@chromium.org
https://chromium.googlesource.com/chromium/src/+/d0f610cfbd1a1d4257ac4464654ebfeedf7e59a6

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

That's an interesting regression... I have no idea why adding a FullscreenControlHost to ExclusiveAccessController can result in ~2MB increment of memory, or how I can reduce that number... Any suggestions?

I'm not sure what the turn-around time is for bugs like this but it is a P2 so we likely have some time to investigate.

We could try disabling / reverting pieces of your change and running the test manually:
https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md#how-do-i-run-the-test

It might also be good to contact the scenario owner to understand what it is doing and when the measurement(s) are taken.

Looking through the CR, I don't see any obvious leaks but maybe it is ARC related?

Does your change bring in any dependencies that might hold onto a chunk of memory?  I was wondering if you loaded an image for the button but looks like that isn't the case.

It looks like the initialization of views::Widget causes a 2MB increment of memory use:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/fullscreen_control/fullscreen_control_popup.cc?l=32&rcl=fc95501fb29e3e7487fff98c17e6aaba1046b330

If I don't initialize the widget or immediately release it, then the 2MB memory use goes away. The memory use only comes from the widget itself. Creating the underlying FullscreenControlView without releasing it doesn't cause the increment.

robliao@ could you recall if you get complains on memory regression when you first integrated FullscreenControlHost into BrowserView? I wonder if Cocoa's widget implementation is more expensive than Views...

Currently FullscreenControlHost is created when the browser enters fullscreen for the first time, and it basically lives until the BrowserView/ExclusiveAccessController gets destructed. It's possible to release it immediately when exiting fullscreen. The problem is that the perf test seems to end before exiting fullscreen, so the memory being released is accounted for.

I didn't receive any complaints for the initial check in to BrowserView.

Is there any opportunity to create it lazily? The code is based off of the FindBar and it does seem there was some effort to make it lazy there due to perf regressions. 
However, that was a time regression rather than a memory regression (https://bugs.chromium.org/p/chromium/issues/detail?id=783350)

> Is there any opportunity to create it lazily?

We can probably make FullscreenControlHost create FullscreenControlPopup lazily. That way it won't take that chunk of memory until it gets triggered by mouse/touch/keyboard. Not sure whether it really addresses the concern but it will at least make the test happy :-/

> However, that was a time regression rather than a memory regression (https://bugs.chromium.org/p/chromium/issues/detail?id=783350)
I got a time regression too when I enabled the FAB on canary ( http://crbug.com/833594 )

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9c514bac3aacc34f0bd9eb45b496158584e3a792

commit 9c514bac3aacc34f0bd9eb45b496158584e3a792
Author: Yuwei Huang <yuweih@chromium.org>
Date: Wed May 16 00:57:23 2018

[Fullscreen Control] Prevent synthesizing mouse move events in test

The event dispatcher synthesizes mouse move events add feeds it to
FullscreenControlHost immediately after the browser enters fullscreen,
which interferes with the tests and makes the tests somewhat flaky.

This CL prevents WindowEventDispatcher from synthesizing mouse move
events. Note that this doesn't effect mouse move events coming from a
real device.

Bug:  841340 
Change-Id: I850e4719e94c6a46dfbf14ae6a542b6ad76c6b7f
Reviewed-on: https://chromium-review.googlesource.com/1058557
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558905}
[modify] https://crrev.com/9c514bac3aacc34f0bd9eb45b496158584e3a792/chrome/browser/ui/views/fullscreen_control/fullscreen_control_view_interactive_uitest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/274455d7f869ad739b016e4eb490c5e3b482cdcf

commit 274455d7f869ad739b016e4eb490c5e3b482cdcf
Author: Yuwei Huang <yuweih@chromium.org>
Date: Wed May 16 03:20:16 2018

[Fullscreen Control] Delay construction of FullscreenControlPopup

The creation of the popup widget causes a ~2MB memory usage increase
in the TrivialFullscreenVideoPageSharedPageState test.

This CL brings down the number by delaying the construction of the
underlying FullscreenControlPopup until it needs to be used.

Bug:  841340 
Change-Id: I5f6f8bb0649b4bd634b9ea67c23fdc0ba0c4d7ae
Reviewed-on: https://chromium-review.googlesource.com/1054547
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558952}
[modify] https://crrev.com/274455d7f869ad739b016e4eb490c5e3b482cdcf/chrome/browser/ui/views/fullscreen_control/fullscreen_control_host.cc
[modify] https://crrev.com/274455d7f869ad739b016e4eb490c5e3b482cdcf/chrome/browser/ui/views/fullscreen_control/fullscreen_control_host.h
[modify] https://crrev.com/274455d7f869ad739b016e4eb490c5e3b482cdcf/chrome/browser/ui/views/fullscreen_control/fullscreen_control_view_interactive_uitest.cc

Should be fixed. https://pinpoint-dot-chromeperf.appspot.com/job/16c07054240000 shows a decrease of memory after my change.