bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Splits action link in Devices table into two separate links: "Inspect"
and "Forget".
Adds functions to add and remove items from the sidebar.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
Adds unregister function to PageManager.
GIFs: https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282, 663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2576603002
Cr-Commit-Position: refs/heads/master@{#443465}
Committed: https://chromium.googlesource.com/chromium/src/+/feda9ca742d2220f0da0d81e17f51a4917375405
Description was changed from
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No detailed service properties are included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
BUG=651282
==========
to
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No detailed service properties are included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
mbrunson
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Description was changed from
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No detailed service properties are included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No detailed service properties are included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
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_rel_ng/builds/334066)
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/349604)
Description was changed from ========== bluetooth: Add device details page with basic properties to internals ...
3 years, 11 months ago
(2017-01-06 02:21:03 UTC)
#11
Description was changed from
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No detailed service properties are included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
mbrunson
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
3 years, 11 months ago
(2017-01-06 20:32:55 UTC)
#12
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/131609) ios-device-xcode-clang on ...
3 years, 11 months ago
(2017-01-06 20:36:22 UTC)
#15
Description was changed from ========== bluetooth: Add device details page with basic properties to internals ...
3 years, 11 months ago
(2017-01-10 00:17:30 UTC)
#22
Description was changed from
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
GIFs:https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
mbrunson
Description was changed from ========== bluetooth: Add device details page with basic properties to internals ...
3 years, 11 months ago
(2017-01-10 00:17:41 UTC)
#23
Description was changed from
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
GIFs:https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
GIFs: https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
mbrunson
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
3 years, 11 months ago
(2017-01-10 00:39:59 UTC)
#24
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
3 years, 11 months ago
(2017-01-10 02:43:41 UTC)
#29
Dry run: Try jobs failed on following builders:
chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
mbrunson
Description was changed from ========== bluetooth: Add device details page with basic properties to internals ...
3 years, 11 months ago
(2017-01-10 20:15:24 UTC)
#30
Description was changed from
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
GIFs: https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Splits action link in Devices table into two separate links: "Inspect"
and "Forget".
Adds functions to add and remove items from the sidebar.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
Adds unregister function to PageManager.
GIFs: https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
mbrunson
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
3 years, 11 months ago
(2017-01-10 22:15:23 UTC)
#31
3 years, 11 months ago
(2017-01-11 00:12:14 UTC)
#34
Dry run: This issue passed the CQ dry run.
scheib
https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode77 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:77: devices.addOrUpdate(deviceDetailsPage.deviceInfo); comment why the addOrUpdate here and below, or ...
3 years, 11 months ago
(2017-01-11 02:16:14 UTC)
#35
3 years, 11 months ago
(2017-01-11 21:22:59 UTC)
#36
https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):
https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:77:
devices.addOrUpdate(deviceDetailsPage.deviceInfo);
On 2017/01/11 02:16:14, scheib wrote:
> comment why the addOrUpdate here and below, or rename addOrUpdate to make it
> clearer why.
Now that I'm looking at this again, this is a bad design. I shouldn't be adding
completely unrelated properties to this data structure.
I've changed this so that the device table tracks what's being inspected
independent of the DeviceInfo structure. It's also easier to follow since that
functionality is explicitly stated in the function name.
https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_details_page.js
(right):
https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:141: if
(!isConnected) this.disconnect();
On 2017/01/11 02:16:14, scheib wrote:
> This condition seems odd here, it's not really part of redrawing is it? Just
> some general maintenance to do if is_gatt_connected ever changes?
It's general maintenance. Redraw is called when the 'DeviceChanged' event fires
from Adapter. If the connection has been dropped, this is the function that
would check the connection state and update the display. I can put this line in
a different function and call it as well if you think that would be easier to
follow.
https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):
https://codereview.chromium.org/2576603002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:16: LINKS: 5,
On 2017/01/11 02:16:14, scheib wrote:
> In screenshot the links column doesn't have a header cell, and looks a bit odd
> IMHO without one. Maybe it doesn't need text as a label, but having the top
> header row be full width would be visually nicer.
Done.
scheib
LGTM
3 years, 11 months ago
(2017-01-11 21:57:37 UTC)
#37
LGTM
mbrunson
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
3 years, 11 months ago
(2017-01-11 22:15:31 UTC)
#38
3 years, 11 months ago
(2017-01-12 00:04:27 UTC)
#44
Dry run: This issue passed the CQ dry run.
dpapad
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.html File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.html#newcode93 chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:93: <button class="disconnect-btn">Disconnect</button> Nit (optional): Drop "-btn" suffix? I don't ...
3 years, 11 months ago
(2017-01-12 18:37:09 UTC)
#45
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
(right):
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:93:
<button class="disconnect-btn">Disconnect</button>
Nit (optional): Drop "-btn" suffix? I don't see any ambiguity.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_details_page.js
(right):
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:53:
this.pageDiv.querySelector('.forget-btn').addEventListener('click',
Nit: Move 'click' parameter to the next line? This violates the "rectangle
rule", see
https://google.github.io/styleguide/jsguide.html#formatting-rectangle-rule.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:65: if
(this.devicePtr) {
Nit(optional): Perhaps compress lines 65-69 as follows?
this.devicePtr !== null ? this.disconnect() : this.connect();
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:72:
this.deviceFieldSet = new object_fieldset.ObjectFieldSet();
This constructor can be cleaned up. Currently it mixes up
initialization/declaration of variables with actual work (registering listeners,
setting up the DOM). Can we separate those as follows?
function DeviceDetailsPage(id, deviceInfo) {
this.foo = 'foo';
this.bar = 'bar';
... // Remaining declarations/initialization of member variables here.
// Set up DOM.
this.pageDiv.appendChild(
document.importNode($('device-details-template').content,
true /* deep */));
this.pageDiv.querySelector('.device-details').appendChild(
this.deviceFieldSet);
// Add event listeners.
this.pageDiv.querySelector('.forget-btn').addEventListener('click', ...);
...
this.redraw();
}
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:93:
adapterBroker.connectToDevice(this.deviceInfo.address).then(
Wrong indent, 4 instead of 2? But also, you can remove one more level of
indentation by chaining as follows
adapter_broker.getAdapterBroker().then(
function(adapterBroker) {
return adapterBroker.connectToDevice(this.deviceInfo.address);
}.bind(this)).then(
function(devicePtr) {
...
return this.devicePtr.getServices();
}.bind(this)).then(
function(response) {
...
}.bind(this)).catch(
function(error) {
...
}.bind(this));
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:116:
SnackbarType.DANGER, 'Retry', function() {
Nit: No need to wrap connect() to a new function anonymous function.
Snackbar.show(
this.deviceInfo.name_for_display + ': ' + error.message,
SnackbarType.DANGER, 'Retry', this.connect.bind(this));
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:151:
'rssi.value': (rssi && rssi.value) || 'Unknown',
Is 0 (zero) a legit value for rssi.value? If so, this will incorrectly display
"Unknown" instead of 0.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:152:
'services.length': (services && services.length) || 'Unknown',
Same here. Should a value of zero be displayed instead of Unknown?
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:167: /**
Fires an 'infochanged' event with the current |deviceInfo| */
@private missing here and elsewhere.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:182: *
@param {Error=} opt_error
Is this still relevant?
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:184:
updateConnectionStatus_: function(status) {
Should there be an early return check?
if (this.status === status)
return;
this.status = status;
...
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:188:
this.disconnectBtn_.disabled = false;
The naming of |disconnectBtn| is confusing. If status is DISCONNECTED, the
button is enabled? I would expect a disconnect button to be disabled if I am
already disconnected.
Should it be renamed to connectBtn?
3 years, 11 months ago
(2017-01-12 22:16:28 UTC)
#46
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
(right):
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:93:
<button class="disconnect-btn">Disconnect</button>
On 2017/01/12 18:37:08, dpapad wrote:
> Nit (optional): Drop "-btn" suffix? I don't see any ambiguity.
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_details_page.js
(right):
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:53:
this.pageDiv.querySelector('.forget-btn').addEventListener('click',
On 2017/01/12 18:37:09, dpapad wrote:
> Nit: Move 'click' parameter to the next line? This violates the "rectangle
> rule", see
> https://google.github.io/styleguide/jsguide.html#formatting-rectangle-rule.
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:65: if
(this.devicePtr) {
On 2017/01/12 18:37:09, dpapad wrote:
> Nit(optional): Perhaps compress lines 65-69 as follows?
>
> this.devicePtr !== null ? this.disconnect() : this.connect();
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:72:
this.deviceFieldSet = new object_fieldset.ObjectFieldSet();
On 2017/01/12 18:37:09, dpapad wrote:
> This constructor can be cleaned up. Currently it mixes up
> initialization/declaration of variables with actual work (registering
listeners,
> setting up the DOM). Can we separate those as follows?
>
> function DeviceDetailsPage(id, deviceInfo) {
> this.foo = 'foo';
> this.bar = 'bar';
> ... // Remaining declarations/initialization of member variables here.
>
> // Set up DOM.
> this.pageDiv.appendChild(
> document.importNode($('device-details-template').content,
> true /* deep */));
> this.pageDiv.querySelector('.device-details').appendChild(
> this.deviceFieldSet);
>
> // Add event listeners.
> this.pageDiv.querySelector('.forget-btn').addEventListener('click', ...);
> ...
>
> this.redraw();
> }
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:93:
adapterBroker.connectToDevice(this.deviceInfo.address).then(
On 2017/01/12 18:37:09, dpapad wrote:
> Wrong indent, 4 instead of 2? But also, you can remove one more level of
> indentation by chaining as follows
>
> adapter_broker.getAdapterBroker().then(
> function(adapterBroker) {
> return adapterBroker.connectToDevice(this.deviceInfo.address);
> }.bind(this)).then(
> function(devicePtr) {
> ...
> return this.devicePtr.getServices();
> }.bind(this)).then(
> function(response) {
> ...
> }.bind(this)).catch(
> function(error) {
> ...
> }.bind(this));
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:116:
SnackbarType.DANGER, 'Retry', function() {
On 2017/01/12 18:37:09, dpapad wrote:
> Nit: No need to wrap connect() to a new function anonymous function.
>
> Snackbar.show(
> this.deviceInfo.name_for_display + ': ' + error.message,
> SnackbarType.DANGER, 'Retry', this.connect.bind(this));
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:151:
'rssi.value': (rssi && rssi.value) || 'Unknown',
On 2017/01/12 18:37:09, dpapad wrote:
> Is 0 (zero) a legit value for rssi.value? If so, this will incorrectly display
> "Unknown" instead of 0.
It's nearly impossible to hit 0 (I think -25 is an amazingly good value for
received signal strength). That being said, I will change this to account for
it. Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:152:
'services.length': (services && services.length) || 'Unknown',
On 2017/01/12 18:37:09, dpapad wrote:
> Same here. Should a value of zero be displayed instead of Unknown?
0 should be shown here if it's defined. I've changed both of these. Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:167: /**
Fires an 'infochanged' event with the current |deviceInfo| */
On 2017/01/12 18:37:09, dpapad wrote:
> @private missing here and elsewhere.
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:182: *
@param {Error=} opt_error
On 2017/01/12 18:37:09, dpapad wrote:
> Is this still relevant?
Removed. Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:184:
updateConnectionStatus_: function(status) {
On 2017/01/12 18:37:09, dpapad wrote:
> Should there be an early return check?
>
> if (this.status === status)
> return;
>
> this.status = status;
> ...
Done.
https://codereview.chromium.org/2576603002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_details_page.js:188:
this.disconnectBtn_.disabled = false;
On 2017/01/12 18:37:09, dpapad wrote:
> The naming of |disconnectBtn| is confusing. If status is DISCONNECTED, the
> button is enabled? I would expect a disconnect button to be disabled if I am
> already disconnected.
>
> Should it be renamed to connectBtn?
Renamed. Done.
dpapad
LGTM with question. https://codereview.chromium.org/2576603002/diff/240001/ui/webui/resources/js/cr/ui/page_manager/page_manager.js File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right): https://codereview.chromium.org/2576603002/diff/240001/ui/webui/resources/js/cr/ui/page_manager/page_manager.js#newcode90 ui/webui/resources/js/cr/ui/page_manager/page_manager.js:90: delete this.registeredPages[page.name.toLowerCase()]; Is it necessary to ...
3 years, 11 months ago
(2017-01-12 22:53:21 UTC)
#47
On 2017/01/12 at 23:09:19, mbrunson wrote: > https://codereview.chromium.org/2576603002/diff/240001/ui/webui/resources/js/cr/ui/page_manager/page_manager.js > File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right): > > https://codereview.chromium.org/2576603002/diff/240001/ui/webui/resources/js/cr/ui/page_manager/page_manager.js#newcode90 ...
3 years, 11 months ago
(2017-01-12 23:48:36 UTC)
#51
On 2017/01/12 at 23:09:19, mbrunson wrote:
>
https://codereview.chromium.org/2576603002/diff/240001/ui/webui/resources/js/...
> File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):
>
>
https://codereview.chromium.org/2576603002/diff/240001/ui/webui/resources/js/...
> ui/webui/resources/js/cr/ui/page_manager/page_manager.js:90: delete
this.registeredPages[page.name.toLowerCase()];
> On 2017/01/12 22:53:21, dpapad wrote:
> > Is it necessary to unregister a page? I am wondering if unregistering a page
can
> > cause an obsolete entry to remain in history. See lines 201-202.
>
> Unregistering a page can create an obsolete entry but I think it's up to
observers to update and handle the history appropriately. PageManager doesn't
set window.history or window.location directly. Because of this, I created a
PageObserver that updates the history stack by changing location.hash after
receiving the new path from PageManager[1]. An event handler determines if this
change is valid and shows the page[2]. If it's invalid, the page is not updated.
>
> [1] (Line 49)
https://codereview.chromium.org/2576603002/diff/240001/chrome/browser/resourc...
> [2] (Line 256)
https://codereview.chromium.org/2576603002/diff/240001/chrome/browser/resourc...
Ok. Thanks for the explanation.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 11 months ago
(2017-01-13 00:44:43 UTC)
#52
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484271846338470, "parent_rev": "591b347c5ffa6c9841a0ae9e08e28b3820ed9641", "commit_rev": "feda9ca742d2220f0da0d81e17f51a4917375405"}
3 years, 11 months ago
(2017-01-13 02:15:27 UTC)
#57
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484271846338470,
"parent_rev": "591b347c5ffa6c9841a0ae9e08e28b3820ed9641", "commit_rev":
"feda9ca742d2220f0da0d81e17f51a4917375405"}
commit-bot: I haz the power
Description was changed from ========== bluetooth: Add device details page with basic properties to internals ...
3 years, 11 months ago
(2017-01-13 02:15:57 UTC)
#58
Message was sent while issue was closed.
Description was changed from
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Splits action link in Devices table into two separate links: "Inspect"
and "Forget".
Adds functions to add and remove items from the sidebar.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
Adds unregister function to PageManager.
GIFs: https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
bluetooth: Add device details page with basic properties to internals page.
Adds DeviceDetailsPage to internals page so users can view all the properties
of the DeviceInfo object. No display of service properties is included in this
patch.
Splits action link in Devices table into two separate links: "Inspect"
and "Forget".
Adds functions to add and remove items from the sidebar.
Adds test infrastructure for Device proxy-based tests.
Adds test for DeviceDetailsPage.
Adds unregister function to PageManager.
GIFs: https://goo.gl/photos/zLiv86gyYmQhRn9w8
BUG=651282,663470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2576603002
Cr-Commit-Position: refs/heads/master@{#443465}
Committed:
https://chromium.googlesource.com/chromium/src/+/feda9ca742d2220f0da0d81e17f5...
==========
commit-bot: I haz the power
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/feda9ca742d2220f0da0d81e17f51a4917375405
3 years, 11 months ago
(2017-01-13 02:15:58 UTC)
#59
Issue 2576603002: bluetooth: Add device details page with basic properties to internals page.
(Closed)
Created 4 years ago by mbrunson
Modified 3 years, 11 months ago
Reviewers: dpapad, scheib
Base URL:
Comments: 32
Chromium Code Reviews
Issue
2576603002:
bluetooth: Add device details page with basic properties to internals page. (Closed)
