[sensors] Add Permission guard to the generic sensor apis.

[sensors][permission] Add Permission guard to the generic sensor apis.

  Authors: Rijubrata Bhaumik, Maksim Sisov

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  
  TODO:
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary changes.


  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[riju_, 2016-10-26 18:32:43]

Description was changed from

==========
[WIP : DO NO LAND] [sensors] Add Permission guard to the generic sensor apis.

  Test page: Test page : https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Comment out some UMA related stuff in the browser side
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  ALS Working with permissions Test page :
https://riju.github.io/permissions.html

  aw permission
  Add CONTENT_SETTINGS_TYPE_SENSORS
  Remove local cache of permissions, use HasPermission() and
GetNextPermissionChange()

  TODO:
  Add tests
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.

  Fix multiple Request/Revoke permissions for sensors.

  Granularity of Sensor permissions. Right now everything is clubbed under
SENSORS permission name

  BUG=606766
==========

to

==========
[WIP : DO NO LAND] [sensors] Add Permission guard to the generic sensor apis.

  Test page: Test page : https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Comment out some UMA related stuff in the browser side
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  ALS Working with permissions Test page :
https://riju.github.io/permissions.html

  aw permission
  Add CONTENT_SETTINGS_TYPE_SENSORS
  Remove local cache of permissions, use HasPermission() and
GetNextPermissionChange()

  TODO:
  Add tests
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.

  Fix multiple Request/Revoke permissions for sensors.

  Granularity of Sensor permissions. Right now everything is clubbed under
SENSORS permission name

  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[riju_, 2016-10-26 18:36:28]

Description was changed from

==========
[WIP : DO NO LAND] [sensors] Add Permission guard to the generic sensor apis.

  Test page: Test page : https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Comment out some UMA related stuff in the browser side
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  ALS Working with permissions Test page :
https://riju.github.io/permissions.html

  aw permission
  Add CONTENT_SETTINGS_TYPE_SENSORS
  Remove local cache of permissions, use HasPermission() and
GetNextPermissionChange()

  TODO:
  Add tests
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.

  Fix multiple Request/Revoke permissions for sensors.

  Granularity of Sensor permissions. Right now everything is clubbed under
SENSORS permission name

  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[WIP : DO NO LAND] [sensors] Add Permission guard to the generic sensor apis.

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Comment out some UMA related stuff in the browser side
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  ALS Working with permissions Test page :
https://riju.github.io/permissions.html

  aw permission
  Add CONTENT_SETTINGS_TYPE_SENSORS
  Remove local cache of permissions, use HasPermission() and
GetNextPermissionChange()

  TODO:
  Add tests
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.

  Fix multiple Request/Revoke permissions for sensors.

  Granularity of Sensor permissions. Right now everything is clubbed under
SENSORS permission name

  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[riju_, 2016-10-26 18:36:29]

rijubrata.bhaumik@intel.com changed reviewers:
+ alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com

[shalamov, 2016-10-27 08:47:25]

https://codereview.chromium.org/2458453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2458453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:40:
private MidiInfo mSensorsInfo;
MidiInfo?

https://codereview.chromium.org/2458453002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:129:
m_permissionService->HasPermission(
What happens if you call start(), then permission popup appears and during that
time you call stop? Right now, after permission is granted you start listening.
I think that is unwanted behavior.

https://codereview.chromium.org/2458453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:138:
m_permissionService->GetNextPermissionChange(
I think logic should be different.

Initially we don't know permission value (done in constructor).
Then, we have to get permission value by calling HasPermission.
Then, once we know permission, we have to continuously subscribe to permission
changes using GetNextPermissionChange.

https://codereview.chromium.org/2458453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:158: void
Sensor::onPermissionUpdate(ExecutionContext* executionContext,
I think permission checking should not be bound to Sensor instance, since if you
create 100 sensors of the same type, all of them would be checking same
permission and subscribing to permission changes. Thus, permission checking
should be moved from Sensor.

https://codereview.chromium.org/2458453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:165: startListening();
ditto, what if user called stop() before permission is granted?

[Mikhail, 2016-11-04 13:06:00]

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:255:
requestPermission(m_executionContext, m_sensorPermission);
here it's a bit late to check permission as sensor is already created.

I'd move permissions check to 'initialize' before we start any other actions.

[shalamov, 2016-11-04 19:00:20]

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:80:
initSensorProxyIfNeeded();
This will request sensor from SensorProvider and initialize proxy with it,    I
think permission should be asked before that (before requesting sensor from
provider).

start() - > Ask permission, so that info about hw capability is not leaked
Granted - > Request sensor
startListening()

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:116:
m_sensorProxy->getNextPermissionChange(scriptState->getExecutionContext(),
This can be moved after HasPermission is completed (to callback from
HasPermission for example).

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:132: auto permissionStatus =
m_sensorProxy->getPermissionStatus();
Is this required for stop()? Should we create issue for spec to clarify it?

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:35: // Get permission
service.
Is this needed in constructor? Also, handleSersonError will go through all
observers (JS instances) and notify them, so line 39 doesn't do anything, since
there can't be any observers at this point.

[Mikhail, 2016-11-07 08:02:02]

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:89: if
(!UserGestureIndicator::consumeUserGesture()) {
must be moved to the beginning.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:100:
m_sensorProxy->requestPermission(scriptState->getExecutionContext(),
return here?

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:134: if (permissionStatus !=
PermissionStatus::GRANTED) {
is it possible that we had permissions to start but do not have them to stop?

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:144:
m_sensorProxy->getNextPermissionChange(scriptState->getExecutionContext(),
this class already has 'getExecutionContext()'

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:228: }
what if permissions are granted? we probably should start sensor than..

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:79:
m_permissionService.reset();
isn't it automatically reset when destructed?

[riju_, 2016-11-09 10:30:34]

Please take a look. Layout tests still to be added.

https://codereview.chromium.org/2458453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2458453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:40:
private MidiInfo mSensorsInfo;
On 2016/10/27 08:47:25, shalamov wrote:
> MidiInfo?

Done.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:80:
initSensorProxyIfNeeded();
On 2016/11/04 19:00:20, shalamov wrote:
> This will request sensor from SensorProvider and initialize proxy with it,   
I
> think permission should be asked before that (before requesting sensor from
> provider).
> 
> start() - > Ask permission, so that info about hw capability is not leaked
> Granted - > Request sensor
> startListening()

If there is no proxy, we can't call proxy->somePermissionMethod(), so putting
the permission checking after initSensorProxyIfNeeded()

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:89: if
(!UserGestureIndicator::consumeUserGesture()) {
On 2016/11/07 08:02:02, Mikhail wrote:
> must be moved to the beginning.

Done.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:100:
m_sensorProxy->requestPermission(scriptState->getExecutionContext(),
On 2016/11/07 08:02:02, Mikhail wrote:
> return here?

Done.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:116:
m_sensorProxy->getNextPermissionChange(scriptState->getExecutionContext(),
On 2016/11/04 19:00:20, shalamov wrote:
> This can be moved after HasPermission is completed (to callback from
> HasPermission for example).

Done.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:132: auto permissionStatus =
m_sensorProxy->getPermissionStatus();
On 2016/11/04 19:00:20, shalamov wrote:
> Is this required for stop()? Should we create issue for spec to clarify it?

Will remove.
https://github.com/w3c/sensors/issues/146

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:134: if (permissionStatus !=
PermissionStatus::GRANTED) {
On 2016/11/07 08:02:02, Mikhail wrote:
> is it possible that we had permissions to start but do not have them to stop?

My mistake, now spec says we don't need permissions to stop()

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:144:
m_sensorProxy->getNextPermissionChange(scriptState->getExecutionContext(),
On 2016/11/07 08:02:02, Mikhail wrote:
> this class already has 'getExecutionContext()'

Moving this to the onPermissionUpdate callback, as alex suggested.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:228: }
On 2016/11/07 08:02:02, Mikhail wrote:
> what if permissions are granted? we probably should start sensor than..

Yes, provided a start() was waiting for permissions to be granted.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:35: // Get permission
service.
On 2016/11/04 19:00:20, shalamov wrote:
> Is this needed in constructor? Also, handleSersonError will go through all
> observers (JS instances) and notify them, so line 39 doesn't do anything,
since
> there can't be any observers at this point.

Moving to Initialize

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:79:
m_permissionService.reset();
On 2016/11/07 08:02:02, Mikhail wrote:
> isn't it automatically reset when destructed?

Done.

https://codereview.chromium.org/2458453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:255:
requestPermission(m_executionContext, m_sensorPermission);
On 2016/11/04 13:05:59, Mikhail wrote:
> here it's a bit late to check permission as sensor is already created.
> 
> I'd move permissions check to 'initialize' before we start any other actions.

Done.

[Mikhail, 2016-11-10 09:30:49]

Reviewed third_party/WebKit/Source/modules/sensor only

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:111: stopListening();
how can be sensor listening in 'idle' or 'errored' state?

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:197:
m_sensorProxy->resetPermissionService();
should be handled by proxy itself

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:212: void
Sensor::onSensorPermissionChanged() {
all this logic should be inside proxy itself

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:36:
m_executionContext(executionContext),
can we pass the "prepared" permissionService here instead of executionContext?

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:39: void
SensorProxy::getNextPermissionChange(ExecutionContext* executionContext,
we don't need this wrapper, it's called only once

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:75: if
(!m_permissionService &&
think we should not init it lazily, see my comment in ctor

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:86:
m_permissionService.reset();
this looks like a fatal error, so service reset is not enough.
handleSensorError() to be invoked

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:129:
setStartPendingPermission(true);
why split 'setStartPendingPermission' and 'requestPermission'?
we can put pending flag inside 'requestPermission'

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:130:
requestPermission(m_executionContext, m_sensorPermission);
here we should interrupt and wait for permissions

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:136:
m_provider->sensorProvider()->GetSensor(m_type, mojo::GetProxy(&m_sensor),
this method should be called *AFTER* permission is granted

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:81:
mojom::blink::PermissionService* getPermissionService(ExecutionContext*);
1) permissionService()
2) why pass |ExecutionContext| there? doesn't proxy have it already?

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:83: void
permissionServiceConnectionError();
name of this function looks like a getter, it's unclear what it's doing

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:84: void
onPermissionUpdate(mojom::blink::PermissionStatus);
does it need to be public? (same comment allies to other introduces methods)

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:88:
mojom::blink::PermissionStatus getPermissionStatus() const {
permissionStatus()

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:91: bool
getStartPendingPermission() const { return m_startPendingPermission; }
bool pendingPermissionRequest() const ?

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:146:
mojom::blink::PermissionStatus m_sensorPermission;
m_permissionStatus

[Mikhail, 2016-11-10 09:48:24]

General comments/recommendations:
IMO permission request should be treated as a part of SensorProxy
initialization: i.e. we must get the permissions granted before we get mojo
proxy for sensor service.

If permission is revoked by user that means a fatal error (all sensor instances
should be stopped and proxy reset).

[riju_, 2016-11-14 14:00:07]

Thanks Mikhail for the comments. PTAL.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:111: stopListening();
On 2016/11/10 09:30:48, Mikhail wrote:
> how can be sensor listening in 'idle' or 'errored' state?

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:197:
m_sensorProxy->resetPermissionService();
On 2016/11/10 09:30:48, Mikhail wrote:
> should be handled by proxy itself

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:212: void
Sensor::onSensorPermissionChanged() {
On 2016/11/10 09:30:48, Mikhail wrote:
> all this logic should be inside proxy itself

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:36:
m_executionContext(executionContext),
On 2016/11/10 09:30:48, Mikhail wrote:
> can we pass the "prepared" permissionService here instead of executionContext?

Yes, done now.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:39: void
SensorProxy::getNextPermissionChange(ExecutionContext* executionContext,
On 2016/11/10 09:30:48, Mikhail wrote:
> we don't need this wrapper, it's called only once

Done. removed.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:75: if
(!m_permissionService &&
On 2016/11/10 09:30:48, Mikhail wrote:
> think we should not init it lazily, see my comment in ctor

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:86:
m_permissionService.reset();
On 2016/11/10 09:30:48, Mikhail wrote:
> this looks like a fatal error, so service reset is not enough.
> handleSensorError() to be invoked

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:129:
setStartPendingPermission(true);
On 2016/11/10 09:30:48, Mikhail wrote:
> why split 'setStartPendingPermission' and 'requestPermission'?
> we can put pending flag inside 'requestPermission'

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:130:
requestPermission(m_executionContext, m_sensorPermission);
On 2016/11/10 09:30:48, Mikhail wrote:
> here we should interrupt and wait for permissions

Acknowledged.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:136:
m_provider->sensorProvider()->GetSensor(m_type, mojo::GetProxy(&m_sensor),
On 2016/11/10 09:30:48, Mikhail wrote:
> this method should be called *AFTER* permission is granted

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:81:
mojom::blink::PermissionService* getPermissionService(ExecutionContext*);
On 2016/11/10 09:30:48, Mikhail wrote:
> 1) permissionService()
> 2) why pass |ExecutionContext| there? doesn't proxy have it already?

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:83: void
permissionServiceConnectionError();
On 2016/11/10 09:30:48, Mikhail wrote:
> name of this function looks like a getter, it's unclear what it's doing

Removing it.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:84: void
onPermissionUpdate(mojom::blink::PermissionStatus);
On 2016/11/10 09:30:48, Mikhail wrote:
> does it need to be public? (same comment allies to other introduces methods)

Done.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:88:
mojom::blink::PermissionStatus getPermissionStatus() const {
On 2016/11/10 09:30:48, Mikhail wrote:
> permissionStatus()

removing it.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:91: bool
getStartPendingPermission() const { return m_startPendingPermission; }
On 2016/11/10 09:30:48, Mikhail wrote:
> bool pendingPermissionRequest() const ?

Not needed now. removing it.

https://codereview.chromium.org/2458453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:146:
mojom::blink::PermissionStatus m_sensorPermission;
On 2016/11/10 09:30:48, Mikhail wrote:
> m_permissionStatus

Done.

[riju_, 2016-11-14 14:01:22]

Patchset #4 (id:60001) has been deleted

[riju_, 2016-11-14 14:11:06]

Patchset #4 (id:80001) has been deleted

[shalamov, 2016-11-14 14:40:07]

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:53:
m_permissionService.reset();
If you reset it here, and access it on initialize in SensorProxy, proxy will
crash.

ASK -> GRANTED -> DENIED -> ASK (crash)

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:88:
m_permissionService->RequestPermission(
Can be garbage, if invalidated.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:164:
m_provider->onPermissionServiceConnectionError();
This will invalidate permission proxy when permission is denied, consecutive
initialize will crash. If I read thru code correctly :D

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:166: m_state =
Initializing;
Can there be a case when we get "GRANTED" and proxy is already initialized?

[Mikhail, 2016-11-15 14:03:18]

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:40:
m_securityOrigin(std::move(origin)) {}
no need to move PassRefPtr, I think PassRefPtr itself is deprecated now.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:87: if
(m_permissionStatus == PermissionStatus::ASK) {
what if (m_permissionStatus != PermissionStatus::ASK) ? We should either
continue initialization or stop with error, depending on its actual value.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:164:
m_provider->onPermissionServiceConnectionError();
we should prepare implementation for more granular permissions,
'handleSensorError' would be enough. Also 'onerror' event with an appropriate
description must be send to the user.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:166: m_state =
Initializing;
we should not assign m_state here. What if SensorProxy is already initialized?

[shalamov, 2016-11-16 11:10:22]

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:77: if
(!UserGestureIndicator::consumeUserGesture()) {
I have two comments for this line:

1. Since it does not need anything from the sensor, it can be moved to the proxy
where all permissions are handled now.

2. Why is this needed?

[riju_, 2016-11-16 12:05:38]

Patchset #5 (id:120001) has been deleted

[riju_, 2016-11-16 12:32:10]

Patchset #5 (id:140001) has been deleted

[riju_, 2016-11-16 12:38:26]

PTAL.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:77: if
(!UserGestureIndicator::consumeUserGesture()) {
On 2016/11/16 11:10:22, shalamov wrote:
> I have two comments for this line:
> 
> 1. Since it does not need anything from the sensor, it can be moved to the
proxy
> where all permissions are handled now.
> 
> 2. Why is this needed?

I am removing it now, as getting "sensor" information might not qualify as very
security-sensitive, as defined in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/UserG...

Let's check if this is required with the security-folks. If needed, I will add
this back to SensorProxy.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:53:
m_permissionService.reset();
On 2016/11/14 14:40:06, shalamov wrote:
> If you reset it here, and access it on initialize in SensorProxy, proxy will
> crash.
> 
> ASK -> GRANTED -> DENIED -> ASK (crash)

Yes, I am removing from initialize() now.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:40:
m_securityOrigin(std::move(origin)) {}
On 2016/11/15 14:03:18, Mikhail wrote:
> no need to move PassRefPtr, I think PassRefPtr itself is deprecated now.

Done.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:87: if
(m_permissionStatus == PermissionStatus::ASK) {
On 2016/11/15 14:03:18, Mikhail wrote:
> what if (m_permissionStatus != PermissionStatus::ASK) ? We should either
> continue initialization or stop with error, depending on its actual value.

Done.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:88:
m_permissionService->RequestPermission(
On 2016/11/14 14:40:06, shalamov wrote:
> Can be garbage, if invalidated.

Acknowledged.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:164:
m_provider->onPermissionServiceConnectionError();
On 2016/11/14 14:40:06, shalamov wrote:
> This will invalidate permission proxy when permission is denied, consecutive
> initialize will crash. If I read thru code correctly :D

My bad, handleSensorError() is called now

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:164:
m_provider->onPermissionServiceConnectionError();
On 2016/11/15 14:03:18, Mikhail wrote:
> we should prepare implementation for more granular permissions,
> 'handleSensorError' would be enough. Also 'onerror' event with an appropriate
> description must be send to the user.

Acknowledged.

https://codereview.chromium.org/2458453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:166: m_state =
Initializing;
On 2016/11/15 14:03:18, Mikhail wrote:
> we should not assign m_state here. What if SensorProxy is already initialized?

Added a check now, so that we don't call GetProxy more than once.

[Mikhail, 2016-11-16 13:04:28]

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:52:
WTF::bind(&SensorProxy::onPermissionUpdate, wrapPersistent(this))));
wrapPersistent will hold SensorProxy from deletion until the callback is
delivered, is it intentional?

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:86: if
(m_permissionStatus == PermissionStatus::ASK) {
nit: switch .. case .. would look better

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:93:
handleSensorError(SecurityError, "Permission not granted.");
NotAllowedError

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:94: }
case for granted permissions?

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:162: void
SensorProxy::SensorPermissionChanged() {
if proxy is not initialized, it should ignore this notification

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:168: m_state =
Initializing;
why? 'Initializing' state should be cause *only* from 'initialize()' method
call.

[shalamov, 2016-11-16 19:03:49]

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:60:
SensorProxy* SensorProviderProxy::createSensor(
I think we should rename it to createSensorProxy. Mikhail wdyt?

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:75:
sensor->handleSensorError(
Two comments:

1. At this point, m_sensors is empty, so, you can't notify sensors about an
error.

2. There is no return, therefore, you create sensor proxy anyways, even there is
no permission service available.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right):

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:8: #include
"core/dom/ExecutionContext.h"
nit: already included in cpp, forward declare

[riju_, 2016-11-18 09:29:35]

Guys, please take a look.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right):

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:8: #include
"core/dom/ExecutionContext.h"
On 2016/11/16 19:03:49, shalamov wrote:
> nit: already included in cpp, forward declare

Acknowledged.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:52:
WTF::bind(&SensorProxy::onPermissionUpdate, wrapPersistent(this))));
On 2016/11/16 13:04:28, Mikhail wrote:
> wrapPersistent will hold SensorProxy from deletion until the callback is
> delivered, is it intentional?

Done.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:86: if
(m_permissionStatus == PermissionStatus::ASK) {
On 2016/11/16 13:04:28, Mikhail wrote:
> nit: switch .. case .. would look better

Done.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:93:
handleSensorError(SecurityError, "Permission not granted.");
On 2016/11/16 13:04:28, Mikhail wrote:
> NotAllowedError

Done.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:94: }
On 2016/11/16 13:04:28, Mikhail wrote:
> case for granted permissions?

Done.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:162: void
SensorProxy::SensorPermissionChanged() {
On 2016/11/16 13:04:28, Mikhail wrote:
> if proxy is not initialized, it should ignore this notification

Done.

https://codereview.chromium.org/2458453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:168: m_state =
Initializing;
On 2016/11/16 13:04:28, Mikhail wrote:
> why? 'Initializing' state should be cause *only* from 'initialize()' method
> call.

Done.

[Mikhail, 2016-11-18 12:37:43]

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:83: return
nullptr;
createSensor should not return nullptr, if there is a fatal error inside
SensorProviderProxy it has a dedicated handler (onSensorProviderConnectionError
-could be renamed to become more generic) for this.

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:44: if
(!isInitialized())
so, any external permission changes could initiate initialization? initialize()
must be called only after it was initially was called from Sensor::start() (i.e.
is current state is 'initializing')

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:50:
handleSensorError(NotAllowedError, "Permission not granted.");
what it if we in the middle of initialization, or initialization is not yet
started? in second case it looks like 'onerror' will be called even before
'start' method call.

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:100:
wrapPersistent(this))));
why to hold proxy instance alive until onPermissionUpdate is called?

[shalamov, 2016-11-18 13:17:45]

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:173: if (!m_sensorProxy)
This is sync operation that is initiated from Sensor::start, so, start will
throw exception (InvalidStateError) and this check will throw async error
NotAllowedError. Looks strange, can we just throw one exception?

[riju_, 2016-11-21 11:10:01]

Patchset #7 (id:200001) has been deleted

[riju_, 2016-11-21 11:23:28]

please take a look.

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:173: if (!m_sensorProxy)
On 2016/11/18 13:17:45, shalamov wrote:
> This is sync operation that is initiated from Sensor::start, so, start will
> throw exception (InvalidStateError) and this check will throw async error
> NotAllowedError. Looks strange, can we just throw one exception?

Done.

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:83: return
nullptr;
On 2016/11/18 12:37:42, Mikhail wrote:
> createSensor should not return nullptr, if there is a fatal error inside
> SensorProviderProxy it has a dedicated handler
(onSensorProviderConnectionError
> -could be renamed to become more generic) for this.

OK, reusing onSensorProviderConnectionError for permission connection failures
also.

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:44: if
(!isInitialized())
On 2016/11/18 12:37:43, Mikhail wrote:
> so, any external permission changes could initiate initialization?
initialize()
> must be called only after it was initially was called from Sensor::start()
(i.e.
> is current state is 'initializing')

Acknowledged.

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:50:
handleSensorError(NotAllowedError, "Permission not granted.");
On 2016/11/18 12:37:42, Mikhail wrote:
> what it if we in the middle of initialization, or initialization is not yet
> started? in second case it looks like 'onerror' will be called even before
> 'start' method call.

Acknowledged.

https://codereview.chromium.org/2458453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:100:
wrapPersistent(this))));
On 2016/11/18 12:37:43, Mikhail wrote:
> why to hold proxy instance alive until onPermissionUpdate is called?

Done.

[Mikhail, 2016-11-21 14:29:41]

looks like various raise conditions are possible

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:173:
provider->createSensor(m_type, origin, createSensorReadingFactory());
nit: std::move(origin)

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:47: if
(isInitializing())
what if this came after permission has already been obtained and we're in the
middle of GetSensor() call? it will be called another time, right?

and if the new value is ASK (and we're still initializing) it will ask for
permission again?

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:105:
handleSensorError(NotAllowedError, "Permission not granted.");
so, state will be still "Initializing"?

[Mikhail, 2016-11-22 21:24:41]

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:46: m_permissionStatus
= status;
looks like 'm_permissionStatus' is not needed and can be removed from the class.

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: if
(m_permissionStatus != PermissionStatus::GRANTED) {
== Denied

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: if
(m_permissionStatus != PermissionStatus::GRANTED) {
can it be ASK again?

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:50:
handleSensorError(NotAllowedError, "Permission not granted.");
return here (no need to subscribe for further notification)

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:60:
m_permissionService->GetNextPermissionChange(
can we drop this subscription at 'handleSensorError'?

[riju_, 2016-11-23 08:51:59]

Patchset #9 (id:260001) has been deleted

[riju_, 2016-11-23 09:02:29]

PTAL.

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:173:
provider->createSensor(m_type, origin, createSensorReadingFactory());
On 2016/11/21 14:29:41, Mikhail wrote:
> nit: std::move(origin)

Acknowledged.

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:105:
handleSensorError(NotAllowedError, "Permission not granted.");
On 2016/11/21 14:29:41, Mikhail wrote:
> so, state will be still "Initializing"?

handleSensorError resets m_state to Uninitialized.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/sensor...

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:46: m_permissionStatus
= status;
On 2016/11/22 21:24:41, Mikhail wrote:
> looks like 'm_permissionStatus' is not needed and can be removed from the
class.

I would like to be always sure that SensorProxy starts with
PermissionStatus::ASK.

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: if
(m_permissionStatus != PermissionStatus::GRANTED) {
On 2016/11/22 21:24:40, Mikhail wrote:
> can it be ASK again?

Acknowledged.

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: if
(m_permissionStatus != PermissionStatus::GRANTED) {
On 2016/11/22 21:24:40, Mikhail wrote:
> can it be ASK again?

Acknowledged.

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:50:
handleSensorError(NotAllowedError, "Permission not granted.");
On 2016/11/22 21:24:41, Mikhail wrote:
> return here (no need to subscribe for further notification)

Acknowledged.

https://codereview.chromium.org/2458453002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:60:
m_permissionService->GetNextPermissionChange(
On 2016/11/22 21:24:41, Mikhail wrote:
> can we drop this subscription at 'handleSensorError'?

resetPermissionService() will now be called if DENIED, which will reset all
callbacks on the permissionService interfacePtr

[shalamov, 2016-11-23 12:37:50]

https://codereview.chromium.org/2458453002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:51:
m_provider->resetPermissionService();
This will invalidate permission service in proxy, and next time start is called,
m_permissionService would be invalid and probably crash.

[Mikhail, 2016-11-28 20:07:53]

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: m_permissionStatus
= status;
Think m_permissionStatus is not needed as not used outside of this method, and
inside as well :) (didn't I leave this comment already?)

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:57: break;
why not return here?

[riju_, 2016-11-29 07:19:47]

comments inline.

https://codereview.chromium.org/2458453002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:51:
m_provider->resetPermissionService();
On 2016/11/23 12:37:49, shalamov wrote:
> This will invalidate permission service in proxy, and next time start is
called,
> m_permissionService would be invalid and probably crash.

Done.

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: m_permissionStatus
= status;
On 2016/11/28 20:07:53, Mikhail wrote:
> Think m_permissionStatus is not needed as not used outside of this method, and
> inside as well :) (didn't I leave this comment already?)

Because I wanted to make sure that, whenever SensorProxy is created, it is
always initialized to ASK.

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:57: break;
On 2016/11/28 20:07:53, Mikhail wrote:
> why not return here?

Because if it is ASK, I would still like to subscribe to
GetNextPermissionChange. 
ASK->GRANTED is a valid change. Only when its DENIED, there is no need to
subscribe to further changes.

[shalamov, 2016-11-29 08:29:11]

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: m_permissionStatus
= status;
On 2016/11/29 07:19:47, riju_ wrote:
> On 2016/11/28 20:07:53, Mikhail wrote:
> > Think m_permissionStatus is not needed as not used outside of this method,
and
> > inside as well :) (didn't I leave this comment already?)
> 
> Because I wanted to make sure that, whenever SensorProxy is created, it is
> always initialized to ASK.

I agree with Mikhail, this line of code and member variable doesn't do anything
and can be removed.

[Mikhail, 2016-11-29 08:46:29]

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:49: m_permissionStatus
= status;
On 2016/11/29 07:19:47, riju_ wrote:
> On 2016/11/28 20:07:53, Mikhail wrote:
> > Think m_permissionStatus is not needed as not used outside of this method,
and
> > inside as well :) (didn't I leave this comment already?)
> 
> Because I wanted to make sure that, whenever SensorProxy is created, it is
> always initialized to ASK.
mm.. how does it help to make sure it is always initialized to ASK?

https://codereview.chromium.org/2458453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:57: break;
On 2016/11/29 07:19:47, riju_ wrote:
> On 2016/11/28 20:07:53, Mikhail wrote:
> > why not return here?
> 
> Because if it is ASK, I would still like to subscribe to
> GetNextPermissionChange. 
> ASK->GRANTED is a valid change. Only when its DENIED, there is no need to
> subscribe to further changes.
This subscription is meaningless as error handler will put proxy to
"uninitialized" state, which cause early return from this method, or am I
missing something?

[riju_, 2016-11-30 12:46:50]

Patchset #11 (id:320001) has been deleted

[riju_, 2016-12-30 10:46:01]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-30 10:46:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-30 11:28:35]

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

[commit-bot: I haz the power, 2016-12-30 11:28:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[riju_, 2017-01-13 16:09:26]

Patchset #12 (id:360001) has been deleted

[riju_, 2017-01-13 16:22:52]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 16:23:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 17:13:17]

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

[commit-bot: I haz the power, 2017-01-13 17:13:18]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[riju_, 2017-01-17 14:29:14]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 14:29:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 15:40:20]

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

[commit-bot: I haz the power, 2017-01-17 15:40:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[riju_, 2017-01-23 11:26:28]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 11:26:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 12:11:01]

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

[commit-bot: I haz the power, 2017-01-23 12:11:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[riju_, 2017-01-23 14:33:23]

Patchset #14 (id:420001) has been deleted

[riju_, 2017-01-23 14:33:37]

Patchset #13 (id:400001) has been deleted

[riju_, 2017-01-23 14:39:39]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 14:39:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 14:41:09]

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

[commit-bot: I haz the power, 2017-01-23 14:41:10]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, no
build URL)

[riju_, 2017-01-30 09:56:50]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 09:57:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 11:50:32]

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

[commit-bot: I haz the power, 2017-01-30 11:50:33]

Dry run: This issue passed the CQ dry run.

[riju_, 2017-02-02 09:00:25]

Description was changed from

==========
[WIP : DO NO LAND] [sensors] Add Permission guard to the generic sensor apis.

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Comment out some UMA related stuff in the browser side
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  ALS Working with permissions Test page :
https://riju.github.io/permissions.html

  aw permission
  Add CONTENT_SETTINGS_TYPE_SENSORS
  Remove local cache of permissions, use HasPermission() and
GetNextPermissionChange()

  TODO:
  Add tests
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.

  Fix multiple Request/Revoke permissions for sensors.

  Granularity of Sensor permissions. Right now everything is clubbed under
SENSORS permission name

  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[WIP : DO NO LAND] [sensors] Add Permission guard to the generic sensor apis.

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Comment out some UMA related stuff in the browser side
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  ALS Working with permissions Test page :
https://riju.github.io/permissions.html

  aw permission
  Add CONTENT_SETTINGS_TYPE_SENSORS
  
  TODO:
  Fix content browser_test in generic_sensor_browsertest.cc
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.


  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[riju_, 2017-02-02 12:35:14]

Description was changed from

==========
[WIP : DO NO LAND] [sensors] Add Permission guard to the generic sensor apis.

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Comment out some UMA related stuff in the browser side
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  ALS Working with permissions Test page :
https://riju.github.io/permissions.html

  aw permission
  Add CONTENT_SETTINGS_TYPE_SENSORS
  
  TODO:
  Fix content browser_test in generic_sensor_browsertest.cc
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.


  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[sensors][permission] Add Permission guard to the generic sensor apis.

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  
  TODO:
  Fix content browser_test in generic_sensor_browsertest.cc
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.


  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[riju_, 2017-02-02 14:27:17]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 14:27:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 14:35:24]

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

[commit-bot: I haz the power, 2017-02-02 14:35:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[riju_, 2017-02-02 16:08:05]

Patchset #15 (id:480001) has been deleted

[riju_, 2017-02-02 16:15:41]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 16:15:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 16:35:15]

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

[commit-bot: I haz the power, 2017-02-02 16:35:17]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[riju_, 2017-02-02 18:26:27]

Patchset #15 (id:500001) has been deleted

[riju_, 2017-02-03 08:04:48]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 08:13:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 08:40:08]

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

[commit-bot: I haz the power, 2017-02-03 08:40:10]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[riju_, 2017-02-03 08:41:01]

Patchset #15 (id:520001) has been deleted

[riju_, 2017-02-03 08:43:00]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 08:43:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 10:55:08]

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

[commit-bot: I haz the power, 2017-02-03 10:55:09]

Dry run: This issue passed the CQ dry run.

[riju_, 2017-02-03 13:29:57]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 13:30:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[riju_, 2017-02-03 13:33:10]

Description was changed from

==========
[sensors][permission] Add Permission guard to the generic sensor apis.

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  
  TODO:
  Fix content browser_test in generic_sensor_browsertest.cc
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.


  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[sensors][permission] Add Permission guard to the generic sensor apis.

  Authors: Rijubrata Bhaumik, Maksim Sisov

  Test page: https://riju.github.io/permissions.html

  Add permission name sensors
  Add permission type in permission manager and util
  Add in generated_resources.grd. Add sensor_permission_context.{h|cc}
  Add PermissionContextBase methods
  Add support for inforbar on Android
  Add permission for sensors in Blink
  Add sensor_permission_context_unittest.cc
  Add/Register in content_settings_registry.
  
  TODO:
  Get "sensor" png files. put in chrome/app/theme/ and make the necessary
changes.


  BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2017-02-03 14:25:40]

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

[commit-bot: I haz the power, 2017-02-03 14:25:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[riju_, 2017-02-03 14:51:05]

Patchset #16 (id:560001) has been deleted

[riju_, 2017-02-03 14:51:48]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 14:51:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[riju_, 2017-02-03 15:22:37]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 15:22:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[riju_, 2017-02-03 16:01:30]

rijubrata.bhaumik@intel.com changed reviewers:
+ raymes@chromium.org

[riju_, 2017-02-03 16:10:29]

@raymes: hi, got to know from alexis and jeffrey that you will be helping us out
with the permissions UI and security review. Please take a look and let me know
what else needs to be done. 
Feel free to add other members of the security team who might be interested. 
FYI : I am using midi's image/gfx as of now.

[commit-bot: I haz the power, 2017-02-03 16:16:30]

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

[commit-bot: I haz the power, 2017-02-03 16:16:32]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[raymes, 2017-02-03 17:18:53]

On 2017/02/03 16:10:29, riju_ wrote:
> @raymes: hi, got to know from alexis and jeffrey that you will be helping us
out
> with the permissions UI and security review. Please take a look and let me
know
> what else needs to be done. 
> Feel free to add other members of the security team who might be interested. 
> FYI : I am using midi's image/gfx as of now.

Thanks Rijubrata! Taking a step back we need to decide on the security UX
requirements before getting into the code specifics.  Let's follow up on that on
the email thread and then we can go from there. Thanks!

[riju_, 2017-02-07 10:17:46]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 10:18:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 12:13:28]

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

[commit-bot: I haz the power, 2017-02-07 12:13:29]

Dry run: This issue passed the CQ dry run.

[riju_, 2017-02-22 15:25:25]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 15:25:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 15:45:48]

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

[commit-bot: I haz the power, 2017-02-22 15:45:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[riju_, 2017-02-23 08:09:15]

Patchset #19 (id:640001) has been deleted

[riju_, 2017-02-23 09:08:42]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 09:09:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 09:12:01]

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

[commit-bot: I haz the power, 2017-02-23 09:12:03]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[riju_, 2017-02-23 09:23:42]

Patchset #19 (id:660001) has been deleted

[riju_, 2017-02-23 13:01:06]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 13:01:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 13:28:24]

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

[commit-bot: I haz the power, 2017-02-23 13:28:26]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[riju_, 2017-02-23 14:00:32]

Patchset #19 (id:680001) has been deleted

[riju_, 2017-02-23 14:04:03]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 14:04:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 15:37:15]

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

[commit-bot: I haz the power, 2017-02-23 15:37:19]

Dry run: This issue passed the CQ dry run.

[riju_, 2017-03-23 08:28:34]

Patchset #20 (id:720001) has been deleted

[riju_, 2017-04-05 18:21:44]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 18:22:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[riju_, 2017-04-05 18:22:45]

Patchset #21 (id:760001) has been deleted

[commit-bot: I haz the power, 2017-04-05 18:26:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2017-04-05 18:30:24]

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

[commit-bot: I haz the power, 2017-04-05 18:30:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)