Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 938896 - Find My Device #16325

Merged
merged 11 commits into from Mar 28, 2014

Conversation

Projects
None yet
4 participants
@eggpi
Copy link
Contributor

eggpi commented Feb 14, 2014

No description provided.

navigator.mozPower.factoryReset();
}, 3000);
} else {
// FIXME can this really happen?

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

Probably not, I would just remove this if statement. I don't think we have any platforms that don't have the mozPower API?

var self = this;

SettingsListener.observe('findmydevice.registered', false, function(value) {
console.log('findmydevice registered: ' + value);

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

Nit: please remove all console.log from the app, or hide them behind a flag. You can also use this: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/dump.js

@@ -1067,6 +1070,8 @@ <h1 data-l10n-id="ime-addkeyboards">Add keyboards</h1>
</div>
<div id="notifications-lockscreen-container">
</div>
<div id="lockscreen-message" hidden>

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

nit: no line break here.


function launchFindMyDevice() {
var app = Applications.getByManifestURL(
'app://findmydevice.gaiamobile.org/manifest.webapp');

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

Use location.protocol instead.

var buttons = document.getElementsByTagName('button');
for (var i = 0; i < buttons.length; i++) {
(function (command) {
buttons[i].addEventListener('click', function() {

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

Use event bubbling on a container instead. You should be able to add the event listener to a single element #buttons, and grab the command by the clicked id.


ring: function fmdc_ring(args, reply) {
var self = this;
var duration = parseInt(args.d);

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

Please add a radix here, I'm assuming you want parseInt(args.d, 10);

word-wrap: break-word;
text-align: center;
overflow-y: auto;
line-height: 1.4;

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

Some units please.

console.error('failed to grab reference to app!');
};

// TODO check command dependencies here?

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

Is there more todo here?

console.log('in factory reset, ready to reset!');
setTimeout(function() {
navigator.mozPower.factoryReset();
}, 3000);

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

nit: I look at this 3000 timeout and get really confused as to why we need it. Could you please add a comment as to why?

function factory_reset() {
if (navigator.mozPower && navigator.mozPower.factoryReset) {
console.log('in factory reset, ready to reset!');
setTimeout(function() {

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

nit: no anonymous functions, please give this a name.


margin: auto;
border-radius: 1rem;
border: 1px solid rgb(230, 230, 230);

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Feb 19, 2014

Member

nit: If no opacity is needed, can we use a hex code instead?

findmydevice-login=Sign In or Create an Account
findmydevice-enable=Enable Find My Device
findmydevice-about=About Find My Device
findmydevice-settings-description=Find My Device allows you to locate, lock or erase yout phone from a web site:

This comment has been minimized.

Copy link
@flodolo

flodolo Mar 26, 2014

Contributor

Typo (yout). Did these strings go through copy review? The login button has an interesting title case.

This comment has been minimized.

Copy link
@eggpi

eggpi Mar 26, 2014

Author Contributor

Thanks for catching that!

These strings are actually temporary, and have been copied from the UX draft at bug 938896. This is all, of course, landing preffed-off and will not be visible by default.

For the record, the review for this whole branch happened at bug 938901 (despite the other bug numbers in the commit messages).

This comment has been minimized.

Copy link
@flodolo

flodolo Mar 26, 2014

Contributor

Would it be possible to get the copy done before landing the actual code? That would make things easier for everyone (e.g. if you land this, and you need to change strings, you'll need new IDs)

vingtetun added a commit that referenced this pull request Mar 28, 2014

Merge pull request #16325 from guilherme-pg/findmydevice
Bug 938896 - Find My Device

@vingtetun vingtetun merged commit 2bbaa0d into mozilla-b2g:master Mar 28, 2014

1 check passed

default The Travis CI build passed
Details

KevinGrandon added a commit to KevinGrandon/gaia that referenced this pull request Mar 28, 2014

Revert "Merge pull request mozilla-b2g#16325 from guilherme-pg/findmy…
…device"

This reverts commit 2bbaa0d, reversing
changes made to 5e89c55.

KevinGrandon added a commit to KevinGrandon/gaia that referenced this pull request Mar 28, 2014

Revert "Merge pull request mozilla-b2g#16325 from guilherme-pg/findmy…
…device"

This reverts commit 2bbaa0d, reversing
changes made to 5e89c55.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.